-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(v9/node): Add shouldHandleError option to fastifyIntegration #17123
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
Conversation
…16845) Resolves: #16819 This requires the Fastify integration to be manually added while initialising Sentry. ```typescript Sentry.init({ integrations: [ Sentry.fastifyIntegration({ shouldHandleError(_error, _request, reply) { return reply.statusCode >= 500; }, }); }, }); ``` If `shouldHandleError` is provided in `setupFastifyErrorHandler`, it overrides `shouldHandleError` provided from `fastifyIntegration`. So this should not break any existing (pre-DC) usage. In summary: - When `setupFastifyErrorHandler` is not used, `shouldHandleError` at `Sentry.init` will be used. - When `setupFastifyErrorHandler` is used but without `shouldHandleError`, `shouldHandleError` at `Sentry.init` will be used. - When in both `setupFastifyErrorHandler` and `Sentry.init` are used with `shouldHandleError`, the one in `setupFastifyErrorHandler` 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.
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.
Both comments from cursor are for what we're doing intentionally. We could have added a warning for the first one maybe. But as that part is for current users so I think we can assume they are using the handler correctly already.
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: Incorrect Start Command Causes Test Failures
The startCommand
in node-fastify-5/playwright.override.config.mjs
is incorrectly set to pnpm start
instead of pnpm start:override
. This prevents the override tests from running app-handle-error-override.ts
and properly testing the shouldHandleError
functionality, unlike Fastify 3 and 4 override configurations.
dev-packages/e2e-tests/test-applications/node-fastify-5/playwright.override.config.mjs#L3-L4
Lines 3 to 4 in c6deb74
const config = getPlaywrightConfig({ | |
startCommand: `pnpm start`, |
Was this report helpful? Give feedback by reacting with 👍 or 👎
size-limit report 📦
|
Backport of #16845
closes #17123