diff --git a/.size-limit.js b/.size-limit.js index b841667d9d89..35f5ad2e9d5b 100644 --- a/.size-limit.js +++ b/.size-limit.js @@ -379,7 +379,7 @@ module.exports = [ }, { 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', ignore: [...builtinModules, ...nodePrefixedBuiltinModules], gzip: true, disablePlugins: ['@size-limit/esbuild'], diff --git a/dev-packages/e2e-tests/test-applications/nextjs-16-standalone/tests/build-output.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-16-standalone/tests/build-output.test.ts index 936ff77b6494..3bd1c5956c6a 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-16-standalone/tests/build-output.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-16-standalone/tests/build-output.test.ts @@ -1,5 +1,6 @@ import { expect, test } from '@playwright/test'; import { findAbsolutePathImports } from '@sentry-internal/test-utils'; +import { spawnSync } from 'child_process'; import * as fs from 'fs'; import * as path from 'path'; @@ -14,19 +15,29 @@ test('emits no absolute-path imports into the relocated standalone output', () = expect(leaks).toEqual([]); }); -// Regression test for the bug this app surfaced (JS-3451): `register.ts` reaches the ESM loader -// hook through `Module.register(...)`, a runtime call `@vercel/nft` cannot trace. The hook was -// therefore never copied into the standalone output, and channel-based instrumentation failed to -// register, silently, behind `debug: true`. Asserting on the traced output rather than on emitted -// telemetry, because Next.js records its server spans through its own wrapping either way, so the -// tests in `standalone.test.ts` stay green even when channel injection is dead. -test('traces the orchestrion ESM loader hook into the standalone output', () => { - const standaloneDir = path.join(process.cwd(), '.next', 'standalone'); - const hookSuffix = path.join('@sentry', 'server-runtime-injection', 'build', 'esm', 'hook.js'); +// Regression test for JS-3451 and JS-4067. `register.ts` hands the ESM loader hook to +// `Module.register()` by runtime path, so the standalone output must ship the hook and everything it +// loads: first `@vercel/nft` didn't trace the hook at all, then Turbopack shipped it without its +// imports. Either way channel injection failed to register, silently, behind `debug: true`. This +// checks the registration marker rather than emitted telemetry, because Next.js records its server +// spans through its own wrapping either way. The runner's Node takes the `Module.registerHooks` path, +// which never loads the hook, so force the `Module.register()` path that Node < 24.13 takes. +test('registers the orchestrion ESM loader hook from the standalone output', () => { + const registerSuffix = path.join('@sentry', 'server-runtime-injection', 'build', 'cjs', 'register.js'); + const registerPath = fs + .readdirSync(path.join(process.cwd(), '.next', 'standalone'), { recursive: true, withFileTypes: true }) + .map(entry => path.join(entry.parentPath, entry.name)) + .find(file => file.endsWith(registerSuffix)); + expect(registerPath).toBeDefined(); - const found = fs - .readdirSync(standaloneDir, { recursive: true, withFileTypes: true }) - .some(entry => entry.isFile() && path.join(entry.parentPath, entry.name).endsWith(hookSuffix)); + const script = ` + require('node:module').registerHooks = undefined; + require(${JSON.stringify(registerPath)}).registerDiagnosticsChannelInjection(); + const marker = globalThis.__SENTRY__?.__SENTRY_ORCHESTRION__ ?? globalThis.__SENTRY_ORCHESTRION__; + process.stdout.write(JSON.stringify(marker)); + `; + const { stdout, status } = spawnSync(process.execPath, ['-e', script], { encoding: 'utf8' }); - expect(found).toBe(true); + expect(status).toBe(0); + expect(JSON.parse(stdout)).toEqual({ runtime: [] }); }); diff --git a/packages/server-runtime-injection/package.json b/packages/server-runtime-injection/package.json index cf44f9bacc4d..0479485a2383 100644 --- a/packages/server-runtime-injection/package.json +++ b/packages/server-runtime-injection/package.json @@ -70,7 +70,7 @@ "lint:fix": "oxlint . --fix --type-aware", "lint": "oxlint . --type-aware", "lint:types": "oxlint src --type-aware --type-check", - "lint:es-compatibility": "es-check es2020 ./build/cjs/*.js && es-check es2020 ./build/esm/*.js --module", + "lint:es-compatibility": "es-check es2020 ./build/cjs/*.js && es-check es2020 ./build/esm/*.js --module --not=./build/esm/hook.js && es-check es2022 ./build/esm/hook.js --module", "test:unit": "vitest run", "test": "vitest run", "test:watch": "vitest --watch", diff --git a/packages/server-runtime-injection/rollup.npm.config.mjs b/packages/server-runtime-injection/rollup.npm.config.mjs index 49f2ddd2ba89..f51c1256c341 100644 --- a/packages/server-runtime-injection/rollup.npm.config.mjs +++ b/packages/server-runtime-injection/rollup.npm.config.mjs @@ -35,7 +35,6 @@ import { makeBaseNPMConfig, makeNPMConfigVariants } from '@sentry-internal/rollu // (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 }; -const commonJSPlugin = commonjs(commonJSOptions); // Replace `debug` with `src/debug.ts`, which logs through the Sentry logger. `debug` and the // `supports-color` it loads read `process.env` at import time, which throws on Deno without env @@ -71,6 +70,15 @@ const guardTracingDump = { }, }; +// The commonjs-converted vendored dependencies import Node builtins as default imports +// (`require('path')` → default import of `path`), and builtins have no `.default` in CJS — so +// builtins need `'default'` interop (the module itself is the default export). +const builtinInterop = id => (id && (id.startsWith('node:') || builtinModules.includes(id)) ? 'default' : 'esModule'); + +// The vendored dependencies import builtins unprefixed (`import … from 'tty'`), which Deno rejects +// and vite-node (Node 26) misresolves as a relative path. Emit them `node:`-prefixed. +const builtinPaths = Object.fromEntries(builtinModules.map(m => [m, `node:${m}`])); + // Bundling files from the repo-root `node_modules` moves rollup's common source ancestor up to the // repo root, so `preserveModules` names our own files `packages/server-runtime-injection/src/...` — // strip that prefix to keep the `build/cjs/register.js` layout the `exports` map points at. A vendored @@ -107,29 +115,42 @@ const orchestrionRuntimeHooks = [ export default [ ...orchestrionRuntimeHooks, + // `register.ts` backs `./register` (the Node SDK `require`s it synchronously from `Sentry.init()`). ...makeNPMConfigVariants( makeBaseNPMConfig({ - // `register.ts` backs `./register` (the Node SDK `require`s it synchronously from - // `Sentry.init()`); `hook.mjs` backs `./hook` (the async `Module.register()` target, loaded on - // Node's ESM loader thread, which cannot resolve bare specifiers into the vendored chunks — but - // relative imports work, so it shares the ESM build's vendored chunks). `./hook` only maps its - // `import` condition, so the `build/cjs` copy is unused. - entrypoints: ['src/register.ts', 'src/hook.mjs'], + entrypoints: ['src/register.ts'], packageSpecificConfig: { - plugins: [debugShim, guardTracingDump, commonJSPlugin, thirdPartyLicensePlugin], + plugins: [debugShim, guardTracingDump, commonjs(commonJSOptions), thirdPartyLicensePlugin], output: { exports: 'named', preserveModules: true, entryFileNames: sanitizedFileNames, - // The commonjs-converted vendored dependencies import Node builtins as default imports - // (`require('path')` → default import of `path`), and builtins have no `.default` in CJS — - // so builtins need `'default'` interop (the module itself is the default export). - interop: id => (id && (id.startsWith('node:') || builtinModules.includes(id)) ? 'default' : 'esModule'), - // The vendored dependencies import builtins unprefixed (`import … from 'tty'`), which Deno - // rejects and vite-node (Node 26) misresolves as a relative path. Emit them `node:`-prefixed. - paths: Object.fromEntries(builtinModules.map(m => [m, `node:${m}`])), + interop: builtinInterop, + paths: builtinPaths, + }, + }, + }), + ), + // `hook.mjs` backs `./hook`, the async `Module.register()` target. `register.ts` hands it over by + // runtime path, so build tracers ship `build/esm/hook.js` but may treat it as an opaque asset + // and skip its imports (Turbopack does). It is therefore one file that imports only Node builtins. + // Its vendored dependencies duplicate the ones in the preserved-modules build above. It keeps the + // real `debug` rather than `debugShim`: `Module.register()` never runs on Deno, and `src/debug.ts` + // would pull in `@sentry/core`'s ESM build. + ...makeNPMConfigVariants( + makeBaseNPMConfig({ + entrypoints: ['src/hook.mjs'], + packageSpecificConfig: { + plugins: [guardTracingDump, commonjs(commonJSOptions), thirdPartyLicensePlugin], + output: { + exports: 'named', + preserveModules: false, + entryFileNames: 'hook.js', + interop: builtinInterop, + paths: builtinPaths, }, }, }), + { emitCjs: false }, ), ]; diff --git a/packages/server-runtime-injection/src/hook.mjs b/packages/server-runtime-injection/src/hook.mjs index e0f187804615..1a6eeacbedb2 100644 --- a/packages/server-runtime-injection/src/hook.mjs +++ b/packages/server-runtime-injection/src/hook.mjs @@ -3,8 +3,7 @@ // `Module.registerHooks` API isn't available). // // `Module.register()` loads its target on Node's ESM loader thread, so the target must be a real, -// on-disk ES module graph — the loader thread cannot resolve bare specifiers into the dependency -// graph this package bundles away, but it can follow relative imports. This shim is therefore an -// entrypoint of the regular ESM build (sharing the vendored dependency chunks) and exposed via the -// `@sentry/server-runtime-injection/hook` subpath. +// on-disk ES module. It is built into a single file that imports only Node builtins (see +// `rollup.npm.config.mjs`), because build tracers ship it without following its imports, and is +// exposed via the `@sentry/server-runtime-injection/hook` subpath. export * from '@apm-js-collab/tracing-hooks/hook.mjs'; diff --git a/packages/server-runtime-injection/src/register.ts b/packages/server-runtime-injection/src/register.ts index b5e73dcfbc05..95c2fa8996e5 100644 --- a/packages/server-runtime-injection/src/register.ts +++ b/packages/server-runtime-injection/src/register.ts @@ -301,8 +301,7 @@ export function registerDiagnosticsChannelInjection(): void { // own imports are missing. let hookPath: string; /*! rollup-include-cjs-only */ - // This file is `build/cjs/register.js`; the loader thread needs the ESM build, which shares - // the vendored dependency chunks. + // This file is `build/cjs/register.js`; the loader thread needs the ESM build. hookPath = join(__dirname, '../esm/hook.js'); /*! rollup-include-cjs-only-end */ /*! rollup-include-esm-only */ @@ -329,7 +328,7 @@ export function registerDiagnosticsChannelInjection(): void { // This registers a small loader hook before the real one. Its `load` sets // `format: 'module'` for every URL inside our ESM build directory and passes all other URLs // through unchanged. Hooks registered earlier also apply when Node loads hook modules - // registered later, so the real hook and its chunks load as ESM even without the marker. + // registered later, so the real hook loads as ESM even without the marker. // The loader is inlined as a `data:` URL because Node always loads those as ESM, so it // cannot fail the same way and needs no file of its own. const esmDirUrl = `${pathToFileURL(dirname(hookPath)).href}/`; diff --git a/packages/server-runtime-injection/test/hook.test.ts b/packages/server-runtime-injection/test/hook.test.ts new file mode 100644 index 000000000000..34ab2202b3d7 --- /dev/null +++ b/packages/server-runtime-injection/test/hook.test.ts @@ -0,0 +1,82 @@ +import { spawnSync } from 'node:child_process'; +import { cpSync, mkdirSync, mkdtempSync, readFileSync, rmSync } from 'node:fs'; +import { builtinModules } from 'node:module'; +import { join } from 'node:path'; +import { parseModule } from 'meriyah'; +import { afterAll, describe, expect, it } from 'vitest'; + +/** + * `register.ts` hands the ESM loader hook to `Module.register()` by runtime path, so build tracers + * ship `build/esm/hook.js` without following its imports (Turbopack on Next.js does this). Anything + * the hook imports beyond Node builtins is then missing on the loader thread. Runs against the + * built files, so it needs the package built. + */ +const buildDir = join(__dirname, '..', 'build'); + +function collectSpecifiers(node: unknown, specifiers: string[]): void { + if (!node || typeof node !== 'object') { + return; + } + if (Array.isArray(node)) { + node.forEach(child => collectSpecifiers(child, specifiers)); + return; + } + const { type, source } = node as { type?: string; source?: { type?: string; value?: unknown } }; + if ( + (type === 'ImportDeclaration' || + type === 'ExportAllDeclaration' || + type === 'ExportNamedDeclaration' || + type === 'ImportExpression') && + source + ) { + specifiers.push(source.type === 'Literal' ? String(source.value) : ''); + } + Object.values(node).forEach(child => collectSpecifiers(child, specifiers)); +} + +describe('the ESM loader hook', () => { + it('imports nothing but Node builtins', () => { + const specifiers: string[] = []; + collectSpecifiers(parseModule(readFileSync(join(buildDir, 'esm', 'hook.js'), 'utf8')), specifiers); + + const builtins = new Set(builtinModules); + const nonBuiltins = specifiers.filter(s => !(s.startsWith('node:') && builtins.has(s.slice('node:'.length)))); + + expect(specifiers.length).toBeGreaterThan(0); + expect(nonBuiltins).toEqual([]); + }); + + 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-')); + cpSync(join(buildDir, 'cjs'), join(copyDir, 'build', 'cjs'), { recursive: true }); + mkdirSync(join(copyDir, 'build', 'esm')); + cpSync(join(buildDir, 'esm', 'hook.js'), join(copyDir, 'build', 'esm', 'hook.js')); + + afterAll(() => { + rmSync(copyDir, { recursive: true, force: true }); + }); + + it('registers it', { timeout: 30_000 }, () => { + // Removing `registerHooks` forces the `Module.register()` path, the one that loads the hook on + // Node < 24.13, whatever Node version runs the test. + const script = ` + require('node:module').registerHooks = undefined; + const { registerDiagnosticsChannelInjection } = require(${JSON.stringify(join(copyDir, 'build', 'cjs', 'register.js'))}); + registerDiagnosticsChannelInjection(); + 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], { + encoding: 'utf8', + }); + + expect(stderr).not.toContain('Failed to register diagnostics-channel injection hooks'); + expect(JSON.parse(stdout)).toEqual({ runtime: [] }); + }); + }); +}); diff --git a/packages/server-runtime-injection/test/nftTrace.test.ts b/packages/server-runtime-injection/test/nftTrace.test.ts index 2aa8ff749dbe..1d7f38d1ed2c 100644 --- a/packages/server-runtime-injection/test/nftTrace.test.ts +++ b/packages/server-runtime-injection/test/nftTrace.test.ts @@ -21,14 +21,11 @@ async function traceFrom(entry: string): Promise> { } describe.each(['cjs', 'esm'])('the %s build', variant => { - it('traces the ESM loader hook and the chunks it imports', { timeout: 60_000 }, async () => { + it('traces the ESM loader hook', { timeout: 60_000 }, async () => { const traced = await traceFrom(`${buildDir}/${variant}/register.js`); expect(traced).toContain(`${buildDir}/esm/hook.js`); // The hook is ESM. Without this marker the loader thread parses it as CommonJS and fails. expect(traced).toContain(`${buildDir}/esm/package.json`); - // The hook re-exports the vendored chain, so its presence alone would not prove the tracer - // followed the hook's own imports rather than copying it as an opaque asset. - expect(traced).toContain(`${buildDir}/esm/vendored/@apm-js-collab/tracing-hooks/hook.js`); }); });