fix(server-utils): Bundle orchestrion runtime chain so it survives downstream tree-shaking - #23668
Closed
mydea wants to merge 3 commits into
Closed
fix(server-utils): Bundle orchestrion runtime chain so it survives downstream tree-shaking#23668mydea wants to merge 3 commits into
mydea wants to merge 3 commits into
Conversation
Add a regression test that re-bundles the built `orchestrion/register` runtime entry the way a downstream bundler does (honouring the package `sideEffects`) and asserts the vendored CJS deps (meriyah, astring, source-map) stay populated. Under `preserveModules`, `@rollup/plugin-commonjs` emits those deps as empty proxy objects populated by cross-module property writes reachable only through bare side-effect imports; downstream tree-shaking drops those writes, so `parse`/`generate`/the SourceMap constructors become `undefined` and every instrumented module crashes when loaded. The test fails on develop and documents the expected behaviour for the fix. Ref #23664 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…wnstream tree-shaking
The `orchestrion/register` and `orchestrion/hook` runtime entrypoints pull in the
vendored transformer chain (`@apm-js-collab/code-transformer` → meriyah, esquery,
astring, source-map). Under `preserveModules`, `@rollup/plugin-commonjs` emits
each named-export CJS dep (meriyah, astring, source-map) as an empty proxy object
(`var meriyah = {}`) that a separate module populates via cross-module property
writes (`meriyah.parse = parse`), reachable only through a bare side-effect import.
When a downstream bundler (Next.js server, serverless, nitro/vite — rollup and
rolldown alike) re-bundles `@sentry/node`, its tree-shaker drops those "unused"
property writes, leaving the proxy empty. At runtime `parse`/`generate`/the
SourceMap constructors are then `undefined`, so any instrumented module throws
`TypeError: parse is not a function` when loaded. esquery escaped this only
because it ships `module.exports = {…}` (a whole default-export object with
nothing to tree-shake off it), which is why the chain was half-shaken rather than
fully removed.
Build those two entrypoints without `preserveModules` so the whole chain lands in
one self-contained shared chunk where each dep's proxy object, its population, and
its consumer are co-located in a single module. Rollup never separates a property
write from a read within one module, so the chain survives downstream tree-shaking
even under this package's `sideEffects: false`. The rest of the package keeps
`preserveModules` for fine-grained consumer tree-shaking.
Root cause: the bug is in the vendored build's output shape, not package metadata
— a `sideEffects` allowlist does not help, because Rollup still applies
statement-level dead-code elimination to the cross-module property writes even in
a module it considers to have side effects. Co-locating the chain is the only
reliable fix.
The shared `rollup-plugin-license` instance now runs across both configs; it
accumulates scanned deps into one Map, so the last build writes the complete
`THIRD-PARTY-LICENSES.txt` union even though the two configs bundle different
subsets.
Fixes #23664
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
size-limit report 📦
|
…ith a long timeout The nx build cache is Node-version-scoped, so on Node versions other than the one the CI build job ran on, `build/` is absent and the test's on-demand `yarn build:transpile` plus the rollup re-bundle exceeded the default 5s test timeout (the test timed out on Node 22/24/26 while passing on Node 20). Move both the build and the re-bundle into `beforeAll` under a single 180s timeout and reduce the test body to fast string assertions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
Superseded by #23675, which pivots from bundling-the-transformer (which added ~61 kB and only partly fixed the issue) to detecting + warning when the runtime hook is bundled and keeping it external. Closing in favour of that approach. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a downstream bundler (Next.js server, serverless, nitro/vite — rollup and rolldown alike) re-bundles
@sentry/node, the vendored orchestrion transformer chain pulled in byorchestrion/register(@apm-js-collab/code-transformer→ meriyah, esquery, astring, source-map) gets partially tree-shaken away. meriyah/astring/source-map are emptied while esquery survives, so at runtimeparse/generate/the SourceMap constructors areundefinedand any instrumented module throwsTypeError: parse is not a functionwhen loaded.Root cause
Under
preserveModules,@rollup/plugin-commonjsemits each named-export CJS dep as an empty proxy object (var meriyah = {}) that a separate module populates via cross-module property writes (meriyah.parse = parse), reachable only through a bare side-effect import. Downstream tree-shakers treat those writes as dead code and drop them, leaving the proxy empty. esquery escaped this only because it shipsmodule.exports = {…}— a whole default-export object with nothing to tree-shake off it — which is why the chain was half-shaken rather than fully removed.The bug lives in the vendored build's output shape, not in package metadata: a
sideEffectsallowlist does not help, because Rollup still applies statement-level dead-code elimination to the cross-module property writes even in a module it considers to have side effects. I verified this both ways before landing on the output-shape fix.Fix
Build the
orchestrion/registerandorchestrion/hookruntime entrypoints withoutpreserveModules, so the whole chain lands in one self-contained shared chunk where each dep's proxy object, its population, and its consumer are co-located in a single module. Rollup never separates a property write from a read within one module, so the chain survives downstream tree-shaking even under this package'ssideEffects: false. The rest of the package keepspreserveModulesfor fine-grained consumer tree-shaking.Only these two entrypoints needed changing — they are the only ones that ship the transformer chain into the user's app. The build-time bundler plugins (
orchestrion/vite, etc.) also carry the chain but run in Node at build time where nothing tree-shakes them, so they are left onpreserveModules. The trade-off is ~1MB of build-time-only duplication in the (already sourcemap-heavy) published tarball; it does not affect consumer app bundle size.The shared
rollup-plugin-licenseinstance now runs across both rollup configs. It accumulates scanned deps into one Map, so whichever build writes last emits the completeTHIRD-PARTY-LICENSES.txtunion even though the two configs bundle different subsets.The added regression test re-bundles the built
registerentry the way a downstream bundler does and asserts the vendored deps stay populated; it fails ondevelopand passes with the fix. The CJS runtime chunk was verified to stay genuine CJS (node: builtins only, norequire(esm)), preserving the original reasons for vendoring.Fixes #23664
🤖 Generated with Claude Code