chore: Remove lazy loading for hook registration - #22443
Merged
Merged
Conversation
Contributor
size-limit report 📦
|
timfish
changed the title
timfish
marked this pull request as ready for review
July 23, 2026 15:56
timfish
requested review from
JPeer264,
chargome,
isaacs,
mydea and
nicohrubec
and removed request for
a team
July 23, 2026 15:56
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2705b96. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
isaacs
requested changes
Jul 23, 2026
isaacs
left a comment
isaacs
left a comment
Member
There was a problem hiding this comment.
The only thing I'd gate on is the license banner preservation, to stay on the right side of the MIT/ISC/Apache/BSD code this bundles.
But, I think it's absolutely the right way to go, and overall works as expected.
| (denoVersion[0] ?? 0) > 2 || | ||
| (denoVersion[0] === 2 && (denoVersion[1] ?? 0) >= 8) | ||
| ); | ||
| if (denoVersionString) { |
Member
There was a problem hiding this comment.
I wonder if the deno test is still necessary, since our minimum Deno version is going to be 2.8.2 in v11?
Comment on lines
+31
to
+36
| // `strictRequires: false`: the default `'auto'` wraps conditionally-required modules (e.g. | ||
| // `debug`'s browser/node split) in lazy initializers exported as `__require` — an export name that | ||
| // downstream re-bundlers mishandle (Turbopack renames it, producing `.require is not a function` | ||
| // crashes in Next.js on Cloudflare). Hoisting is safe here: the vendored graph is closed (nothing | ||
| // optional/missing) and has no require cycles that depend on lazy evaluation. | ||
| const commonJSOptions = { transformMixedEsModules: true, requireReturnsDefault: 'auto', strictRequires: false }; |
Member
There was a problem hiding this comment.
(low/safety) It'd be good to have a smoke test that verifies this, if only to prevent us from upgrading and breaking something down the road, if meriyah or apm-js-collab stuff ever violates that assumption.
isaacs
approved these changes
Jul 23, 2026
Member
There was a problem hiding this comment.
LGTM! Checking with our oss legal brain trust now about the third-party-licenses.txt thing, but we can always patch it (or back this out) before cutting a v11 release, if they find any problems with the approach.
Update: Sentry counsel says this is the way, so we're good. Third-party bundle is obvious in the build artifact, and the single collected license declaration is standard practice. 👍
andreiborza
added a commit
that referenced
this pull request
Jul 24, 2026
develop's hook-registration rework (#22443) resolves the bundled-pnpm specifier via absolute-path webpack externals and dropped the tracingHooksDir option from registerDiagnosticsChannelInjection and the _sentryOrchestrionTracingHooksDir build var. Remove the now-broken nextjs workaround that relied on them; node's init registers the hooks and the externals keep them resolvable.
andreiborza
added a commit
that referenced
this pull request
Jul 24, 2026
develop's hook-registration rework (#22443) resolves the bundled-pnpm specifier via absolute-path webpack externals and dropped the tracingHooksDir option from registerDiagnosticsChannelInjection and the _sentryOrchestrionTracingHooksDir build var. Remove the now-broken nextjs workaround that relied on them; node's init registers the hooks and the externals keep them resolvable.
andreiborza
added a commit
that referenced
this pull request
Jul 24, 2026
develop's hook-registration rework (#22443) resolves the bundled-pnpm specifier via absolute-path webpack externals and dropped the tracingHooksDir option from registerDiagnosticsChannelInjection and the _sentryOrchestrionTracingHooksDir build var. Remove the now-broken nextjs workaround that relied on them; node's init registers the hooks and the externals keep them resolvable.
andreiborza
added a commit
that referenced
this pull request
Jul 24, 2026
develop's hook-registration rework (#22443) resolves the bundled-pnpm specifier via absolute-path webpack externals and dropped the tracingHooksDir option from registerDiagnosticsChannelInjection and the _sentryOrchestrionTracingHooksDir build var. Remove the now-broken nextjs workaround that relied on them; node's init registers the hooks and the externals keep them resolvable.
andreiborza
added a commit
that referenced
this pull request
Jul 24, 2026
develop's hook-registration rework (#22443) resolves the bundled-pnpm specifier via absolute-path webpack externals and dropped the tracingHooksDir option from registerDiagnosticsChannelInjection and the _sentryOrchestrionTracingHooksDir build var. Remove the now-broken nextjs workaround that relied on them; node's init registers the hooks and the externals keep them resolvable.
andreiborza
added a commit
that referenced
this pull request
Jul 24, 2026
develop's hook-registration rework (#22443) resolves the bundled-pnpm specifier via absolute-path webpack externals and dropped the tracingHooksDir option from registerDiagnosticsChannelInjection and the _sentryOrchestrionTracingHooksDir build var. Remove the now-broken nextjs workaround that relied on them; node's init registers the hooks and the externals keep them resolvable.
andreiborza
added a commit
that referenced
this pull request
Jul 24, 2026
develop's hook-registration rework (#22443) resolves the bundled-pnpm specifier via absolute-path webpack externals and dropped the tracingHooksDir option from registerDiagnosticsChannelInjection and the _sentryOrchestrionTracingHooksDir build var. Remove the now-broken nextjs workaround that relied on them; node's init registers the hooks and the externals keep them resolvable.
andreiborza
added a commit
that referenced
this pull request
Jul 24, 2026
develop's hook-registration rework (#22443) resolves the bundled-pnpm specifier via absolute-path webpack externals and dropped the tracingHooksDir option from registerDiagnosticsChannelInjection and the _sentryOrchestrionTracingHooksDir build var. Remove the now-broken nextjs workaround that relied on them; node's init registers the hooks and the externals keep them resolvable.
andreiborza
added a commit
that referenced
this pull request
Jul 24, 2026
develop's hook-registration rework (#22443) resolves the bundled-pnpm specifier via absolute-path webpack externals and dropped the tracingHooksDir option from registerDiagnosticsChannelInjection and the _sentryOrchestrionTracingHooksDir build var. Remove the now-broken nextjs workaround that relied on them; node's init registers the hooks and the externals keep them resolvable.
andreiborza
added a commit
that referenced
this pull request
Aug 27, 2026
…roduction dependencies (#23667) Backport of: #22443 Closes: #23611 `@sentry/server-utils` shipped the orchestrion bundler plugins and a full JavaScript parser toolchain (meriyah, esquery, astring, source-map) as production dependencies, so every consumer of `@sentry/node` installed roughly 5 MB of code that no runtime path reaches unless they opt into diagnostics-channel injection. The orchestrion runtime chain is now compiled into this package's own build, and all `@apm-js-collab/*` packages plus meriyah become devDependencies. Production dependencies are down to `@sentry/conventions` and `@sentry/core`. ## Differences to the original PR - `packages/server-utils/src/orchestrion/runtime/register.ts`: kept `RegisterDiagnosticsChannelInjectionOptions` and the optional parameter on `registerDiagnosticsChannelInjection()`, now deprecated and ignored. The original deletes both; on v10 the option is reachable through the public `experimentalUseDiagnosticsChannelInjection(options)`, so removing it would be a compile error for anyone passing `tracingHooksDir`. - `packages/node/src/sdk/experimentalUseDiagnosticsChannelInjection.ts`: same reason, the optional parameter stays. - `packages/server-utils/src/orchestrion/runtime/register.ts`: kept v10's loader-thread guard (`isMainThread` / `parentPort`), which landed after the v10 branch point and is not in the original. - `packages/nextjs/src/config/withSentryConfig/getFinalConfigObjectUtils.ts` and `getFinalConfigObject.ts`: removed `maybeAddOutputFileTracingIncludes` plus its tests in `packages/nextjs/test/config/withSentryConfig.test.ts`. That `@vercel/nft` force-include is a v10-only workaround for meriyah's `module-sync` exports map (#23034), which bundling removes. `develop` never carried it, so this brings both branches to the same state. --------- Co-authored-by: Tim Fish <tim@timfish.uk>
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
2 participants
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.
This PR simplifies the
register.tshook registration to remove all the lazy loading. The clankers seem to always resort to lazy loading but pushed hard enough they will find better alternatives 🤣Once the lazy loading was removed there were a number of other things required to get this actually passing all the e2e tests:
@apm-js-collab/*, meriyah, esquery, …) into@sentry/server-utils' build (all now devDependencies). The dist only has relative requires, which removes two classes of breakage:require(esm): the CJS build is now genuine CJS — fixes AWS Lambda (--no-experimental-require-module) and theModule.registerloader-thread crash on Node 22.15–24.12module-synccondition (Traced file set depends on the Node version running the trace (module-syncgated on build-time Node >= 22) vercel/nft#603, Nested dependency silently missing when its exports map has noimportcondition nitrojs/nitro#4456) — no runtime package resolution left to get wrongModule.registerasync hook is now a self-referenced entrypoint of our own ESM build (@sentry/server-utils/orchestrion/hook), sharing the vendored chunks./orchestrion/webpack-loader), resolved by self-reference since the@apm-js-collabpackages are no longer installeddefaultinterop; default-only ESM deps needrequireReturnsDefault: 'auto')@sentry/server-utilsexternal via absolute-path externals soregister.tsstays innode_modulesand itsModule.registerself-reference resolves under pnpm — also removes thetracingHooksPathworkaround@sentry/nodesize limit by ~40kb — the lazy loading previously hid this code from the size report--require#22513