-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(node): Add shouldHandleError
option to fastifyIntegration
#16845
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
size-limit report 📦
|
Yeah good point. I wouldn't mind changing the name to |
shouldHandleError
option to fastifyIntegration
shouldHandleDiagnosticsChannelError
option to fastifyIntegration
31d50b1
to
fc2159e
Compare
Thinking about this some more, can we make this work in a way that also works in v3/v4? By storing this in some variable that the setupErrorHandler accesses...? IMHO this would be the cleanest solution, from an API perspective - why should I know or care, as a user, that this uses diagnostics channel under the hood 😅 I think the way to go is, to store & expose this method on the integration itself, making this the owner of the handler. Then it can be set in the integration, or in the handler (which updates it on the integration), and it's used both by the v5 as well as the v3/v4 approach. |
b9178db
to
8054696
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Unhandled Promise Rejection Masks Test Failures
The test creates an unhandled promise rejection. It attempts to verify an error is not captured by throwing an error in an unawaited .then()
callback if the error is unexpectedly captured. This results in an unhandled promise rejection, obscuring the actual test failure.
dev-packages/e2e-tests/test-applications/node-fastify-3/tests/errors.test.ts#L36-L39
sentry-javascript/dev-packages/e2e-tests/test-applications/node-fastify-3/tests/errors.test.ts
Lines 36 to 39 in 8054696
errorEventPromise.then(() => { | |
throw new Error('This error should not be captured'); | |
}); |
dev-packages/e2e-tests/test-applications/node-fastify-4/tests/errors.test.ts#L58-L61
sentry-javascript/dev-packages/e2e-tests/test-applications/node-fastify-4/tests/errors.test.ts
Lines 58 to 61 in 8054696
errorEventPromise.then(() => { | |
throw new Error('This error should not be captured'); | |
}); |
dev-packages/e2e-tests/test-applications/node-fastify-5/tests/errors.test.ts#L36-L39
sentry-javascript/dev-packages/e2e-tests/test-applications/node-fastify-5/tests/errors.test.ts
Lines 36 to 39 in 8054696
errorEventPromise.then(() => { | |
throw new Error('This error should not be captured'); | |
}); |
Bug: Playwright Config Uses Incorrect Start Script
The node-fastify-5
Playwright override config (playwright.override.config.mjs
) incorrectly uses pnpm start
instead of pnpm start:override
. This is inconsistent with Fastify v3 and v4 configurations, causing override tests to run the default app.ts
instead of src/app-handle-error-override.ts
, which prevents proper testing of the shouldHandleError
functionality.
dev-packages/e2e-tests/test-applications/node-fastify-5/playwright.override.config.mjs#L3-L4
Lines 3 to 4 in 8054696
const config = getPlaywrightConfig({ | |
startCommand: `pnpm start`, |
Was this report helpful? Give feedback by reacting with 👍 or 👎
shouldHandleDiagnosticsChannelError
option to fastifyIntegration
shouldHandleError
option to fastifyIntegration
@mydea, @AbhiPrasad - I updated the PR, it should now work for all Fastify versions. |
Resolves: #16819
This requires the Fastify integration to be manually added while initialising Sentry.
If
shouldHandleError
is provided insetupFastifyErrorHandler
, it overridesshouldHandleError
provided fromfastifyIntegration
. So this should not break any existing (pre-DC) usage.In summary:
setupFastifyErrorHandler
is not used,shouldHandleError
atSentry.init
will be used.setupFastifyErrorHandler
is used but withoutshouldHandleError
,shouldHandleError
atSentry.init
will be used.setupFastifyErrorHandler
andSentry.init
are used withshouldHandleError
, the one insetupFastifyErrorHandler
is used.Works for all Fastify versions supported (v3, v4, v5)
Note: E2E tests for this now requires to test overrides, so they need to be run twice with both overriden and non-overriden applications. Not the prettiest solution, but I could not figure out an alternative.