feat(express)!: Remove shouldHandleError from setupExpressErrorHandler - #23732
Conversation
size-limit report 📦
|
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
This accomplishes the stated goal to make expressIntegration() the single owner of the error filter and match the fastify pattern.
Most of the comments below are pretty minor cleanup opportunities. The type name confusion between core and server-utils might be worth cleaning up now, but can also be done in a subsequent follow-up.
The bigger opportunity for improvement would be adding an E2E test for the new runtime path added. That is: the integration is registered with a custom shouldHandleError, the orchestrion transform is not active, so the deprecated middleware captures, and reads the predicate off the integration. Nothing exercises that end to end, might be worth just telling an agent to add an e2d for that, so it's part of our covered contract.
That said, it seems to be working, so probably fine either way.
| /* oxlint-disable typescript/no-deprecated -- deprecated Express error-handler exports, kept until the next major */ | ||
| export { expressErrorHandler, setupExpressErrorHandler } from './integrations/express/error-handler'; | ||
| export type { ExpressHandlerOptions } from './integrations/express/types'; | ||
| export type { ExpressIntegrationOptions } from './integrations/express/types'; |
There was a problem hiding this comment.
This is a bit minor, but could be confusing. @sentry/core already exports a different ExpressIntegrationOptions (in packages/core/src/server-exports.ts line 23, defined at packages/core/src/integrations/express/types.ts lines 147-153.)
The core version is deprecated, and doesn't have shouldHandleError. This one isn't actually accessible, since it's not re-exported by @sentry/node.
This can certainly be cleaned up in a followup, but I think it would be best to coalesce these into a single type (either defined in core, or here, but not both).
There was a problem hiding this comment.
Yes, this makes sense and can be done in a follow-up 👍
| @@ -53,7 +53,7 @@ export { vercelAIIntegration } from './integrations/vercel-ai'; | |||
| export { expressIntegration } from './integrations/express'; | |||
| /* oxlint-disable typescript/no-deprecated -- deprecated Express error-handler exports, kept until the next major */ | |||
There was a problem hiding this comment.
I don't think any deprecated items are being pulled in here, are they? It's the one in core that's deprecated, this one isn't.
There was a problem hiding this comment.
Those exports are deprecated as well
| if (shouldHandleError(error)) { | ||
| // `shouldHandleError` is configured on `expressIntegration()` only. Without the integration | ||
| // registered, the default predicate applies. | ||
| if (shouldCaptureError(getExpressIntegration()?.getShouldHandleError(), error)) { |
There was a problem hiding this comment.
| if (shouldCaptureError(getExpressIntegration()?.getShouldHandleError(), error)) { | |
| if (shouldCaptureError(getExpressIntegration()?.getShouldHandleError?.(), error)) { |
Unlikely edge case, but if the Express integration name is used by something that doesn't have this method, it'd throw.
| + integrations: [ | ||
| + Sentry.expressIntegration({ | ||
| + shouldHandleError(error) { | ||
| + return (error.statusCode ?? 500) >= 400; |
There was a problem hiding this comment.
| + return (error.statusCode ?? 500) >= 400; | |
| + return Number(error.statusCode ?? 500) >= 400; |
low: error.statusCode can be a string.
| * 5xx errors, and treats an error without a resolvable status as a 500. Errors | ||
| * carrying a 3xx/4xx status are skipped (client errors / redirects). | ||
| */ | ||
| export function defaultShouldHandleError(error: MiddlewareError): boolean { |
There was a problem hiding this comment.
low: Does anything else use this? If it's only used internally, I don't think we still need to export it.
# Conflicts: # MIGRATION.md # packages/server-utils/test/integrations/express-error-handler.test.ts
3 participants
shouldHandleErrornow lives only onexpressIntegration().setupExpressErrorHandlerandexpressErrorHandlerno longer accept the option, and theExpressHandlerOptionstype is removed.This finishes #23464: the integration owns error capture, so it should own the filter too. Fastify and Hapi already work this way.
The deprecated
setupExpressErrorHandlerstill captures when the integration cannot (setups where orchestrion does not work). But filtering keeps working on theexpressIntegration. It captures 5xx errors and errors without a status, and cannot be filtered. Filtering is the reason to migrate toexpressIntegration().First merge this one: