Repository navigation
Conversation
…dleRequest Adds a Web Streams variant of createSentryHandleRequest, accepted through an overload and exported from the cloudflare entry, and handles a </head> tag split across chunks in both the Node and Web transformers. Closes getsentry#25168
chargome
left a comment
chargome
left a comment
There was a problem hiding this comment.
Overall looks good already, will trigger ci
| body.allReady.then(clearAbortTimer, clearAbortTimer); | ||
|
|
||
| if (isBot || isSpaMode) { | ||
| await body.allReady.catch(() => undefined); |
There was a problem hiding this comment.
Is this catch intended? If so why?
There was a problem hiding this comment.
Not intentional, removed. It would have hidden fatal allReady errors and sent bots/SPA a 200 with a broken body. It now rejects like a shell error does. Added a test.
| import type { | ||
| HandleRequestWithMiddleware, | ||
| HandleRequestWithoutMiddleware, | ||
| SentryWebStreamHandleRequestOptions, | ||
| } from './createSentryHandleRequestWebStream'; |
There was a problem hiding this comment.
These imports should move to some shared type file instead. Node should not import from the web types
There was a problem hiding this comment.
Moved to handleRequestTypes.ts, so Node now only imports types from there. It still imports the Web handler at runtime for the dispatch.
…types Resolve the trace meta tags when injectTraceMetaTags is called instead of inside the stream transform. Runtimes such as workerd run the transform outside the request's async context, where no trace data is available, so the tags were silently dropped. Also let allReady rejections propagate for bots and SPA mode, and move the handler types shared by the Node and Web Streams handlers into their own module.
|
The hydrogen e2e failure was a regression from this PR: the trace tags were resolved inside the stream transform, which workerd runs outside the request context. They're now resolved up front. Fixed in 424465d. |
This branch has not been deployed
2 participants
Adds a Web Streams variant of
createSentryHandleRequest, so apps onrenderToReadableStream(non-Node runtimes, or Node withunstable_enableNodeReadableStream) no longer have to hand-composewrapSentryHandleRequestandinjectTraceMetaTags.Following the constraints from the discussion on the issue to keep it non-breaking:
SentryHandleRequestOptionsand the Node path are untouched. The Web options are a separate type, accepted through an overload and only used whenrenderToReadableStreamis passed in.</head>split across chunks was previously missed by both transformers. A shared helper now holds back only a trailing partial</head>(rather than a fixed number of characters) and flushes it at the end of the stream, so it adds as little streaming delay as possible. It is used bygetMetaTagTransformer(Node) andinjectTraceMetaTags(Web).injectTraceMetaTagsmoved to a Node-free module and is still re-exported from/cloudflare, so the public API is unchanged.react-router-8-frameworkcloudflare e2e entry now uses the new API instead of a hand-written copy of it.Fixes #25168