Repository navigation
fix(server-runtime-injection): Bundle the ESM loader hook into one self-contained file - #26271
sentry-junior[bot] wants to merge 7 commits into
Conversation
…lf-contained file `register.ts` hands `build/esm/hook.js` to `Module.register()` by runtime path. Turbopack ships that file into the Next.js server trace but does not follow its imports, so on Node < 24.13 the loader thread failed to find the vendored chunks (and `@sentry/core`'s ESM build, imported by the `debug` shim) and registration warned on every cold start. The hook is now built as a single file that imports only Node builtins. Its `debug` shim is a no-op: the loader thread has its own `globalThis`, so the Sentry logger there is never enabled. Fixes #26121 Co-Authored-By: Francesco Gringl-Novy <francesco.novy@sentry.io>
size-limit report 📦Compared: cc1fc15 (PR test merge using develop at d6d803c) Against: d6d803c (develop) · base run #65636 ✅ Baseline matches: this diff contains only this PR's changes.
|
|
One or more gzipped bundles increased by more than 500 bytes. If this increase is intentional, add the Accept Bundlesize Increase label to this PR to rerun and accept the check. |
The no-op shim only existed to avoid `src/debug.ts`. The hook never runs on Deno, so the original reason for `debugShim` doesn't apply, and bundling `debug` keeps `DEBUG=@apm-js-collab/*` working on the loader thread. The hook build now feeds the shared license plugin so debug, ms, supports-color and has-flag are listed in THIRD-PARTY-LICENSES.txt.
Optional E2E testsSome optional checks were not successful. They do not block the merge. 12 not successful, 45 successful. See the run.
Successful checks
|
…/import Co-Authored-By: Francesco Gringl-Novy <francesco.novy@sentry.io>
The registration test fails whenever the hook or anything it loads is missing, so the file-exists check adds nothing. Assert on the registration marker instead of the warning text, which #26272 rewords.
timfish
left a comment
timfish
left a comment
There was a problem hiding this comment.
This doesn't add 1.6MB to all server SDKs dependencies, it actually adds 3.4MB with the additional sourcemap file!
Hold off on this for a bit, I think I have an alternative!
|
I think this is a better solution with no downsides? |
| makeBaseNPMConfig({ | ||
| entrypoints: ['src/hook.mjs'], | ||
| packageSpecificConfig: { | ||
| plugins: [guardTracingDump, commonjs(commonJSOptions), thirdPartyLicensePlugin], |
There was a problem hiding this comment.
The hook bundle runs debug's browser build at load and touches localStorage.
debug/src/index.js picks ./browser.js or ./node.js with a runtime if. With strictRequires: false, @rollup/plugin-commonjs hoists both requires, so the bundled hook.js evaluates browser.js too (see browser.exports near line 466 of the built build/esm/hook.js). browser.js runs exports.storage = localstorage(), which reads the global localStorage. On Node versions where Web Storage is on by default, that read emits a process warning:
(node:94064) ExperimentalWarning: localStorage is not available because --localstorage-file was not provided.
Reproduced on Node 26.5 with the forced Module.register() path. When I replaced return localStorage; with return undefined; in the built hook, the warning went away. In real use, Node 26 takes the registerHooks path, so it is not affected. But hasStableSyncModuleHooks() (packages/server-runtime-injection/src/register.ts line 34) sends Node 25.0.x down the Module.register() path, and Node 25 turns on Web Storage by default. I did not have a Node 25.0 binary to confirm. Node 22 with --experimental-webstorage may also hit it (not verified). This is the kind of "warning on every cold start" the PR sets out to remove, and develop did not have it, because debugShim kept the real debug out of the hook.
Suggestion: in the hook build only, resolve debug straight to its Node entry, for example a resolveId plugin with order: 'pre' that maps debug to require.resolve('debug/src/node.js') (the same pattern as debugShim). That also drops browser.js from the bundle. Add an assertion to test/hook.test.ts that loading the hook writes nothing to stderr, or at least that the built file does not contain localStorage.
| plugins: [guardTracingDump, commonjs(commonJSOptions), thirdPartyLicensePlugin], | ||
| output: { | ||
| exports: 'named', | ||
| preserveModules: false, |
There was a problem hiding this comment.
The PR says the package grows "by about 1.6 MB". I packed this branch and compared it with the published 11.12.0:
| 11.12.0 (npm) | this PR (local pack) | |
|---|---|---|
| unpacked size | 6.96 MB | 10.43 MB |
build/esm/hook.js |
n/a | 0.54 MB |
build/esm/hook.js.map |
n/a | 2.94 MB |
The new map holds 2.3 MB of mappings and 0.59 MB of sourcesContent. The tarball is about 1.0 MB. Develop has moved since 11.12.0, so the numbers are not exact, but the growth is about +3.5 MB unpacked. This package is a dependency of @sentry/node, so every Node SDK install pays it, and serverless users with size limits (Lambda layers, Vercel functions that trace the whole build/esm dir) can feel it.
Suggestion: fix the number in the PR description. Then decide whether the hook needs a full source map. Node only uses it with --enable-source-maps, and errors from the loader thread are rare. Options: sourcemapExcludeSources: true for the hook output, or sourcemap: false for this config only. Either is a one-line change in the output block.
| { | ||
| name: '@sentry/node/import (ESM hook with diagnostics-channel injection)', | ||
| path: ['packages/server-runtime-injection/build/esm/hook.js', 'packages/node/build/import-hook.mjs'], | ||
| path: 'packages/node/build/import-hook.mjs', |
There was a problem hiding this comment.
nothing measures the loader-thread code any more
Before, the entry measured hook.js plus import-hook.mjs. Now it measures only import-hook.mjs, and register.ts reaches the hook through join(), which webpack does not follow. So the 540 kB hook, which every Module.register() user loads, has no size check at all. If a future bump of @apm-js-collab/* or a new dependency doubles it, CI will not notice. The PR's "81.85 kB vs 82.32 kB" improvement comes from not counting that code, not from loading less.
Suggestion: add a second entry, for example @sentry/server-runtime-injection hook (loader thread) with path: 'packages/server-runtime-injection/build/esm/hook.js' and the same ignore list, so changes to that file stay visible. (The bot comment "One or more gzipped bundles increased by more than 500 bytes" does not match the current table, so it is probably from an earlier push.)
| const hookSpecifier = hookFound ? pathToFileURL(hookPath).href : '@sentry/server-runtime-injection/hook'; | ||
|
|
||
| if (hookFound) { | ||
| // Only `build/esm/package.json` marks the hook and its chunks as ESM. Deployments can drop |
There was a problem hiding this comment.
| // Only `build/esm/package.json` marks the hook as ESM. Deployments can drop |
| // Built from `join()` rather than `new URL('./hook.js', import.meta.url)` because webpack | ||
| // reads that second form as an asset reference: it copies the hook next to the app bundle | ||
| // without its vendored chunks, which would leave the loader thread importing a file whose | ||
| // own imports are missing. |
There was a problem hiding this comment.
Still referring to chunks here, looks like it was missed in the other edits.
Suggestion: Either drop the "without its vendored chunks" reasoning or restate why join() is still preferred. If the only reason left is webpack's asset copy, it may now be safe to switch to new URL(), but that changes where the hook comes from in bundled apps, so it is a separate decision and out of scope here.
| describe('when a deployment ships only the hook from the ESM build', () => { | ||
| // The copy sits inside this package so `register.js` still resolves `@sentry/*` from its | ||
| // `node_modules`. | ||
| const copyDir = mkdtempSync(join(__dirname, '.hook-only-')); |
There was a problem hiding this comment.
mkdtempSync(join(__dirname, '.hook-only-')) runs while vitest collects the file, not in a beforeAll. It creates test/.hook-only-XXXX/ with a full copy of build/cjs. If the run is killed or collection fails after that line, the folder stays behind. It is not in .gitignore, so it shows up in git status and can be committed by mistake. It is also created when the test is filtered out.
Suggestion: create it in beforeAll. Better, put it in os.tmpdir() and give the child NODE_PATH set to this package's node_modules. I checked that build/cjs/register.js resolves @sentry/core and @sentry/server-utils that way from /tmp.
| import('data:text/javascript,export default 1').then(() => { | ||
| const marker = globalThis.__SENTRY__?.__SENTRY_ORCHESTRION__ ?? globalThis.__SENTRY_ORCHESTRION__; | ||
| process.stdout.write(JSON.stringify(marker)); | ||
| }); | ||
| `; | ||
|
|
||
| const { stdout, stderr } = spawnSync(process.execPath, ['--no-experimental-detect-module', '-e', script], { |
There was a problem hiding this comment.
import('data:text/javascript,export default 1').then(...)waits for an import before it reads the marker. A broken hook fails insideModule.register()on the spot (verified above), and the e2e test reads the marker right away. The wait is not needed, and the two tests now do the same check in two different ways.--no-experimental-detect-moduleis there, I think, to copy Vercel's setting so that the missingbuild/esm/package.jsonmatters and thedata:format loader gets exercised. That is a good thing to test, but nothing says so.
Suggestion: drop the import() wait so the script matches the e2e one, and add a short comment on the flag. Or state the assumption if the wait is needed for a reason I missed.
| const thirdPartyLicensePlugin = license({ | ||
| thirdParty: { | ||
| includePrivate: false, | ||
| output: { | ||
| file: 'build/THIRD-PARTY-LICENSES.txt', | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
This shared license plugin instance is now load-bearing.
All three configs (register CJS, register ESM, hook ESM) use the same thirdPartyLicensePlugin object and write the same build/THIRD-PARTY-LICENSES.txt. The file is right only because that one instance collects dependencies across all builds. The final file lists debug, ms, supports-color and has-flag (hook only) and module-details-from-path (register only), so this works today. If someone changes it to one license() call per config, which is what the PR just did for commonjs(), the last build wins and some notices go missing with no error.
Suggestion: add one line to the comment above thirdPartyLicensePlugin saying that the instance must be shared so the file covers every build. A cheap guard is a unit test that reads build/THIRD-PARTY-LICENSES.txt and checks for debug and module-details-from-path.
yeah if that works that seems much better, closing this in favor of that then! thanks for the reviews ❤️ |
…CJS build (#26278) On Node < 24.13, the CJS build of `register.ts` passed `build/esm/hook.js` to `Module.register()`. Turbopack ships that file in the Next.js server trace but does not follow its imports, so the loader thread could not find `build/esm/vendored/**` or the ESM build of `@sentry/core`. Registration then warned on every cold start. The CJS build now registers `build/cjs/hook.js`, which the package already built but did not use. That file only requires chunks that `register.js` requires too, so a tracer that ships a working `register.js` always ships them. Node accepts a CJS module as a `Module.register()` target. This is an alternative to #26271: the package does not grow and no Next.js config is needed. The `data:` URL loader that forced the ESM format for `build/esm` is removed. It only helped the CJS build load the ESM hook when a deployment dropped `build/esm/package.json`, and the CJS build no longer loads anything from `build/esm`. The ESM build still registers the ESM hook. The new `test/hook.test.ts` registers the hook from a copy that ships only `build/cjs`. The `nextjs-16-standalone` build-output test (taken from #26271) forces the `Module.register()` path against the real Turbopack standalone output. Both fail on `develop` with the error from the issue. Closes #26121 --------- Co-authored-by: isaacs <i@izs.me>
…se the deployment is missing a file (#26272) **Why:** Follow-up to #26271. When `Module.register()` fails with `ERR_MODULE_NOT_FOUND`, the always-on warning used to just repeat Node's "Cannot find module …". That reads like an SDK bug, when the real cause is that a build tracer didn't ship all of `@sentry/server-runtime-injection`. The warning now says so. I checked on Node 22 that `error.code` survives the trip back from the loader thread. The warning still fires once per process: issue #26121 asks for it to be quiet, but staying silent would hide that channel integrations aren't recording anything (see #25677). **How to review:** It's a single branch in the `catch` block of `registerDiagnosticsChannelInjection()`, plus one unit test. Refs #26121 <!-- junior-request-attribution:start --> via **Francesco Gringl-Novy**. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:slack%3AD0CA8FXFXAS%3A1794383205.954799 --> -- [View Session](https://junior-prod.sentry.dev/conversations/slack%3AD0CA8FXFXAS%3A1794383205.954799) [[Sentry]](https://sentry.sentry.io/explore/conversations/slack%3AD0CA8FXFXAS%3A1794383205.954799/?project=4510944073809921) <!-- junior-session-footer:end --> --------- Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: Francesco Gringl-Novy <francesco.novy@sentry.io>
…se the deployment is missing a file (#26272) **Why:** Follow-up to #26271. When `Module.register()` fails with `ERR_MODULE_NOT_FOUND`, the always-on warning used to just repeat Node's "Cannot find module …". That reads like an SDK bug, when the real cause is that a build tracer didn't ship all of `@sentry/server-runtime-injection`. The warning now says so. I checked on Node 22 that `error.code` survives the trip back from the loader thread. The warning still fires once per process: issue #26121 asks for it to be quiet, but staying silent would hide that channel integrations aren't recording anything (see #25677). **How to review:** It's a single branch in the `catch` block of `registerDiagnosticsChannelInjection()`, plus one unit test. Refs #26121 <!-- junior-request-attribution:start --> via **Francesco Gringl-Novy**. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:slack%3AD0CA8FXFXAS%3A1794383205.954799 --> -- [View Session](https://junior-prod.sentry.dev/conversations/slack%3AD0CA8FXFXAS%3A1794383205.954799) [[Sentry]](https://sentry.sentry.io/explore/conversations/slack%3AD0CA8FXFXAS%3A1794383205.954799/?project=4510944073809921) <!-- junior-session-footer:end --> --------- Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: Francesco Gringl-Novy <francesco.novy@sentry.io>
Why: On Node < 24.13,
register.tspassesbuild/esm/hook.jstoModule.register()using a path computed at runtime. Turbopack includes that file in the Next.js server trace but treats it as an opaque asset and doesn't follow its imports. So the loader thread can't findbuild/esm/vendored/**, and also can't find@sentry/core's ESM build, whichsrc/debug.tsimports. Registration then warns on every cold start. I reproduced this locally with thenextjs-16-standaloneapp (Next 16.4.3, Turbopack, pnpm, Node 22) and got the exact error from the issue. The standalone output containedbuild/esm/hook.jsand nothing else frombuild/esm.Instead of trying to make every tracer follow the hook's imports, the hook is now a single file that imports only
node:builtins. Non-obvious decisions:debugpackage instead ofsrc/debug.ts, which imports@sentry/core.debugShimonly exists becausedebugreadsprocess.envat import time, which throws on Deno, andModule.register()never runs there. The Sentry logger couldn't print on the loader thread anyway, since that thread has its ownglobalThis. I checked this on Node 22: afterdebug.enable()on the main thread,debug.isEnabled()in aModule.register()loader still returnsfalse. NowDEBUG=@apm-js-collab/*shows the hook's messages there. Its license notices (debug, ms, supports-color, has-flag) are added toTHIRD-PARTY-LICENSES.txt.hook.jsand once in the preserved-modules chunks thatregister.jsuses for the CJS_compilepatch. The published package grows by about 1.6 MB, mostly the newhook.js.map. This package is server-only. At runtime nothing extra loads: the loader thread always parsed its own copy of the transformer.@sentry/node/importsize-limit entry now measures onlyimport-hook.mjs.hook.jsruns on the separate loader thread, so bundling the two together made webpack count the transformer twice. Locally the entry measures 81.85 kB, vs 82.32 kB ondevelop.vendored/**, which thees-checkglob doesn't cover. Now it sits in top-levelbuild/esm/hook.js, solint:es-compatibilitychecks that file at es2022 and everything else at es2020 as before.How to review: Start with
rollup.npm.config.mjs. The newtest/hook.test.tschecks that the built hook imports only builtins, and that registration succeeds when a deployment ships nothing from the ESM build excepthook.js. Both tests fail ondevelopwith the error from the issue. The newnextjs-16-standalonebuild-output test does the same check against the real Turbopack and webpack standalone output. It forces theModule.register()path because the runner's Node usesregisterHooks. Without the fix, it fails with the issue's error. With the fix, it passes for both bundlers.Unsure about: Whether the extra package size is acceptable. The quieter warning for missing hook files comes in a separate PR.
Closes #26121
via Francesco Gringl-Novy.
--
View Session [Sentry]