Repository navigation
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
size-limit report 📦
|
2a97f05 to
01d3ad1
Compare
a38c9f8 to
b12d5d7
Compare
b12d5d7 to
42650ee
Compare
b7712e1 to
1a7314a
Compare
d93c0f9 to
a6ecc88
Compare
1a7314a to
424ddfa
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 424ddfa. Configure here.
4cf95b6 to
a01699a
Compare
73ba6de to
eb9579a
Compare
a01699a to
4316c1b
Compare
…injection The orchestrion registration previously mapped one integration per module, so `nitroServerTimingIntegration` was anchored on `unstorage` while `nitroIntegration` took `h3`. `unstorage` is practically always evaluated in a Nitro app, but it is not the guaranteed HTTP module, so a Nitro app that did not load it would silently fail to register Server-Timing (breaking pageload trace propagation) on a bundler-only SDK like `@sentry/cloudflare`. Let a module register several integrations, and anchor both nitro integrations on `h3` — the module every HTTP-serving Nitro app loads. The global orchestrion marker now stores a Set of factories per module; the injected snippet emits one registration call per export; and cloudflare instantiates all of them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ctories The prior test only exercised the init-time path with a Set of factories. Add a test that emits `orchestrion.module-injected` for a module holding a Set, so the late-registration listener's Set-unwrapping is covered with more than one factory too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Emitting one `orchestrionModuleInjected` call per export made the helper merge factories across calls, but every instrumented file of a package injects its own fresh thunk for the same integration. Packages split across files (pg, amqplib, ...) therefore accumulated duplicate factories and were upgraded to a Set despite having a single integration, breaking the plain-factory shape kept for older readers. The snippet now passes all of a module's factories in a single call, and the helper treats each call as the module's complete list and replaces the entry: one factory stays a plain factory, several become a Set. This also announces `h3` once per file instead of once per integration. Adds a transform test for the multi-export `h3` injection. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4316c1b to
800b14e
Compare
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
I think we can remove a layer of complexity by settling on a single shape here, since we pin versions and don't normally support skew anyway, but this is also fine as-is.
| * instantiates them. | ||
| */ | ||
| integrations?: Map<string, () => Integration>; | ||
| integrations?: Map<string, (() => Integration) | Set<() => Integration>>; |
There was a problem hiding this comment.
This dual factory | Set shape does not give the compatibility it claims, for the only module that uses it. I think we should pick one and simplify it.
The marker field has shipped since #22094 as Map<string, () => Integration>. This PR keeps a plain factory for single-integration modules "so an older reader stays compatible".
But h3 is now always a Set. A released @sentry/cloudflare reader does [...registered.values()].map(factory => factory()), which throws TypeError: factory is not a function on the Set. That throw happens inside getDefaultIntegrations during init(), so a Nitro app with version skew between the injected @sentry/server-utils and @sentry/cloudflare would fail at init, not just miss an integration.
@sentry/cloudflare pins @sentry/server-utils to an exact version, so skew needs two installed copies (for example an app-level @sentry/server-utils dependency at another version), which would be very surprising, and we could easily say is unsupported. But then the compat argument is weak for every module, and the dual shape is complexity for nothing. If skew is a real concern (which I'm not convinced it is), the current design would still break for exactly the Nitro case this PR targets.
Suggestion: Treat writer/reader skew as unsupported, as the exact pin already implies. Store one shape always (Set or array) for every module. Drop the [first, ...rest] branching in moduleInjected.ts, the instanceof Set checks in both baseSdk.ts readers, and the compat wording in this comment.
There was a problem hiding this comment.
yeah good point, I rewrote it to expect an array only!
Writer/reader version skew is already unsupported (`@sentry/cloudflare` pins `@sentry/server-utils` exactly), and the dual factory | Set shape did not keep older readers working for `h3` anyway. Always store the module's factory list as an array so the readers no longer need to branch on the shape. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…le-injected snippet Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: isaacs <i@izs.me>
|
Superseded by #25349, which merges the two nitro integrations into a single |
3 participants
Stacked on #24861.
Addresses review feedback on the orchestrion registration:
nitroServerTimingIntegrationwas anchored onunstorage(because the registration mapped one integration per module andh3was taken bynitroIntegration).unstorageis practically always evaluated in a Nitro app, but it isn't the guaranteed HTTP module — so a Nitro app that didn't load it would silently fail to register the Server-Timing headers on a bundler-only SDK like@sentry/cloudflare, breaking pageload trace propagation with no error.This lets a single module register multiple integrations and anchors both nitro integrations on
h3— the module every HTTP-serving Nitro app loads.channel-integration-definitions.ts: both nitro entries →h3;subscriberExportForModule→subscriberExportsForModule(returns all exports for a module).@sentry/server-utilsand the reading SDK is unsupported (@sentry/cloudflarepins@sentry/server-utilsexactly), so there is no older-reader shape to preserve.config/nitro.tsdrops the now-unnecessaryunstorageanchor.Only affects the Cloudflare (bundler-only) path — on node/deno/bun these integrations are registered statically.
🤖 Generated with Claude Code