chore: Remove lazy loading for hook registration#22443
Conversation
size-limit report 📦
|
timfish
changed the title
![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.
| }; | ||
| import ModulePatch from '@apm-js-collab/tracing-hooks'; | ||
| import { initialize, load, resolve, createDiagnosticsPort } from '@apm-js-collab/tracing-hooks/hook-sync.mjs'; | ||
| import { setDiagnosticsHook } from '@apm-js-collab/tracing-hooks/lib/diagnostics.js'; |
There was a problem hiding this comment.
Eager orchestrion load on import
High Severity
register.ts now statically imports the full @apm-js-collab/tracing-hooks / transform chain. That module is pulled in by experimentalUseDiagnosticsChannelInjection, which @sentry/node re-exports statically, so any unbundled import/require of @sentry/node eagerly parses the vendored orchestrion runtime even when the experimental API is never called. Previously those deps stayed behind a runtime require() inside register().
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2705b96. Configure here.
There was a problem hiding this comment.
This will not be experimental soon!
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
isaacs
left a comment
isaacs
left a comment
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) { |
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?
| const sanitizedFileNames = info => | ||
| `${info.name.replace(/^packages\/server-utils\/src\//, '').replace(/node_modules/g, 'vendored')}.js`; | ||
|
|
There was a problem hiding this comment.
If we're vendoring these, we're going to need to ensure that the license headers don't get dropped. Checking that now locally...
There was a problem hiding this comment.
Yeah, looks like our build strips those headers, which is a compliance gap. So, we're going to need to fold those up into either an inline banner in the bundled code, or a notice in our license that we're shipping MIT, BSD, Apache 2, and ISC code, along with the relevant copyright headers.
I think there's a plugin that does this? rollup-plugin-license maybe?
| // `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 }; |
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.
2 participants
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