fix(nextjs): Do not exit process when errors bubble up while additional uncaughtException-handlers are registered
#6138
Conversation
…ws mimicing native exit behaviour
…al `uncaughtException`-handlers are registered
…o lforst-fix-nextjs-quitting-on-aborted-requests
lforst
changed the base branch from
master
to
lforst-mimic-native-behaviour-onuncaught-exception
packages/nextjs/src/index.server.ts
Outdated
| integrations = addOrUpdateIntegration(nativeBehaviourOnUncaughtException, integrations, { | ||
| _options: { exitEvenWhenOtherOnUncaughtExceptionHandlersAreRegistered: false }, | ||
| }); |
There was a problem hiding this comment.
I actually really don't like this. Seems very fragile.
There was a problem hiding this comment.
It's a little hacky, it's true. It predates my work a few months ago cleaning up how default integrations interact with custom ones, and I thought about changing it then, but I couldn't come up with a better solution to the problem of sometimes needing to merge our default options with user-provided options in the same integration, and sometimes needing to force certain integrations to be there even if the user has overridden the other defaults, without changing the entire system.
If you have a good idea here, I'm all ears. That said, if we have the right tests in place, any changes to the underlying integration class's options should cause the tests to fail, so I don't think it's actually that brittle as long as we're testing well.
…uncaught-exception' into lforst-fix-nextjs-quitting-on-aborted-requests
| const nativeBehaviourOnUncaughtException = new Integrations.OnUncaughtException(); | ||
| integrations = addOrUpdateIntegration(nativeBehaviourOnUncaughtException, integrations, { | ||
| _options: { exitEvenIfOtherHandlersAreRegistered: false }, | ||
| }); |
There was a problem hiding this comment.
I'm a little confused by this. The point of addOrUpdateIntegration is to say, "here's the integration to add if no instance is already there, but if an instance is there, update it with these options." This means that the integration to add should have the same options as the the ones which are being updated.
There was a problem hiding this comment.
Oh, I assumed the options didn't matter since the UncaughtException integration is part of the default integrations anyways but you're right - we should add the options in case the users override the integration.
This PR updates the Next.js SDK so that it won't quit the Next.js server when an error bubbles up to global scope while there are additional
uncaughtExceptionhandlers registered (e.g. by Next.js itself when middleware is provided).It uses an option introduced in #6137.