Skip to content

Commit ebe8fdc

Browse files
authored
fix(server-utils): Bundle the CommonJS dependencies of instrumented packages on Vite 8 (#25523)
We put pg into ssr.noExternal and that bundles `pg` and changes its CJS imports. In the second bundling step, Rolldown in Vite 8 emits `__require("pg-types")` for the dependency imports of pg (see [their migration guide](https://vite.dev/guide/migration#require-calls-for-externalized-modules) mentioning `esmExternalRequirePlugin` - also check the linked rolldown docs). As `__require` comes from `module.createRequire`, the bundlers think it's a normal function variable, and not an import. Therefore, a sub-dependency like `pg-types` cannot get into the final bundle. The plugin now bundles exactly these `require()` calls. The user's `ssr.external` entries and optional dependencies stay external. We didn't use `esmExternalRequirePlugin`, because it breaks function-exported CommonJS on Node < 23. Stacked on #25359. Fixes #25470 --- **Same problem: `__require()` of an external package stays in bundled CommonJS** - nitrojs/nitro#4171 - rolldown/rolldown#10643 - rolldown/rolldown#9407 **Workarounds and fixes in other tools** - nitrojs/nitro#4365 - vitejs/vite#22618 - vitejs/vite#23759 - vitejs/vite#21969 **Existing Sentry issues** - #24775 - #25339
1 parent 6380873 commit ebe8fdc

6 files changed

Lines changed: 352 additions & 1 deletion

File tree

‎dev-packages/e2e-tests/test-applications/tanstackstart-react/package.json‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
"test:build:tunnel-static": "pnpm install && E2E_TEST_TUNNEL_ROUTE_MODE=static E2E_TEST_DSN=http://public@localhost:3031/1337 pnpm build",
1515
"test:build:tunnel-custom": "pnpm install && E2E_TEST_CUSTOM_TUNNEL_ROUTE=1 pnpm build",
1616
"test:build:tunnel-object": "pnpm install && E2E_TEST_TUNNEL_ROUTE_MODE=object E2E_TEST_DSN=http://public@localhost:3031/1337 pnpm build",
17+
"test:build:hoisted": "pnpm install --config.node-linker=hoisted && pnpm build",
1718
"test:build-latest": "pnpm add @tanstack/react-start@latest @tanstack/react-router@latest nitro@latest vite@latest @vitejs/plugin-react-swc@latest vite-tsconfig-paths@latest && pnpm install && pnpm build",
1819
"test:assert:proxy": "pnpm test && TEST_ENV=development pnpm playwright test db-drivers",
1920
"test:assert": "pnpm test:assert:proxy",
@@ -51,6 +52,10 @@
5152
},
5253
"sentryTest": {
5354
"variants": [
55+
{
56+
"label": "tanstackstart-react (hoisted)",
57+
"build-command": "pnpm test:build:hoisted"
58+
},
5459
{
5560
"label": "tanstackstart-react (tunnel-generated)",
5661
"build-command": "pnpm test:build:tunnel-generated",
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import { readdirSync, readFileSync } from 'node:fs';
2+
import path from 'node:path';
3+
import { expect, test } from '@playwright/test';
4+
import { bundleReferencesModule } from '@sentry-internal/test-utils';
5+
6+
// Nitro can't follow the `__require()` calls that Vite 8 emits for external requires in bundled
7+
// CommonJS, so the deployed server fails with `Cannot find module`. Only flat installs (the hoisted
8+
// variant) keep the drivers' dependencies external.
9+
// https://vite.dev/guide/migration#require-calls-for-externalized-modules
10+
test('bundles the CommonJS dependencies of the instrumented drivers', () => {
11+
const root = path.join(process.cwd(), '.output/server');
12+
const serverBundle = readdirSync(root, { recursive: true, encoding: 'utf8' })
13+
.filter(file => /\.m?js$/.test(file) && !file.startsWith('node_modules'))
14+
.map(file => readFileSync(path.join(root, file), 'utf8'))
15+
.join('\n');
16+
const driverDependencies = ['sqlstring', 'bignumber.js', 'denque', 'cluster-key-slot', 'standard-as-callback'];
17+
18+
const externalRequires = [...serverBundle.matchAll(/__require\(["']([^"']+)["']\)/g)].map(match => match[1]);
19+
20+
expect(bundleReferencesModule(serverBundle, 'mysql'), 'mysql must be bundled to be instrumented').toBe(false);
21+
expect(externalRequires.filter(name => driverDependencies.includes(name))).toEqual([]);
22+
});
Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
import { isPlainObject } from '@sentry/core';
2+
import { readFileSync } from 'node:fs';
3+
import { createRequire, isBuiltin } from 'node:module';
4+
import { isAbsolute } from 'node:path';
5+
import type { Plugin } from 'vite';
6+
7+
type ResolveIdHandler = Extract<NonNullable<Plugin['resolveId']>, (...args: never[]) => unknown>;
8+
type ResolveIdParameters = Parameters<ResolveIdHandler>;
9+
10+
const NODE_MODULES = '/node_modules/';
11+
12+
/**
13+
* Creates a `resolveId` handler that bundles the external packages that bundled CommonJS `require()`s.
14+
*
15+
* Rolldown keeps these calls as `__require()`, which a second bundler over the server output can't follow:
16+
* - https://vite.dev/guide/migration#require-calls-for-externalized-modules
17+
* - https://rolldown.rs/in-depth/bundling-cjs#require-external-modules
18+
*
19+
* Only handles requires from packages bundled for orchestrion, and from packages bundled because of them.
20+
*/
21+
export function createExternalRequireResolver(
22+
moduleNames: readonly string[],
23+
): (this: ThisParameterType<ResolveIdHandler>, ...args: ResolveIdParameters) => Promise<string | null> {
24+
const bundledPackagesByEnvironment = new Map<string, Set<string>>();
25+
26+
return async function resolveExternalRequire(source, importer, options) {
27+
if (
28+
!this.meta.rolldownVersion ||
29+
this.environment.mode !== 'build' ||
30+
options.kind !== 'require-call' ||
31+
!importer ||
32+
!isPackageSpecifier(source)
33+
) {
34+
return null;
35+
}
36+
37+
const bundledPackages = bundledPackagesByEnvironment.get(this.environment.name) ?? new Set(moduleNames);
38+
bundledPackagesByEnvironment.set(this.environment.name, bundledPackages);
39+
40+
const owner = ownerPackage(importer);
41+
const name = packageName(source);
42+
if (
43+
!owner ||
44+
!bundledPackages.has(owner.name) ||
45+
isUserExternal(this.environment.config.resolve.external, source, name) ||
46+
isOptionalDependency(owner.dir, name)
47+
) {
48+
return null;
49+
}
50+
51+
const resolved = await this.resolve(source, importer, { ...options, skipSelf: true });
52+
if (!resolved) {
53+
return null;
54+
}
55+
// Vite bundles it anyway (e.g. under pnpm), so its own requires count too.
56+
if (!resolved.external) {
57+
bundledPackages.add(name);
58+
return null;
59+
}
60+
61+
const file = resolveWithNode(source, importer);
62+
if (file) {
63+
bundledPackages.add(name);
64+
}
65+
return file ?? null;
66+
};
67+
}
68+
69+
function isPackageSpecifier(source: string): boolean {
70+
return !isBuiltin(source) && !isAbsolute(source) && !/^[.\0]/.test(source);
71+
}
72+
73+
// `pg-types/lib/x` → `pg-types`, `@scope/pkg/x` → `@scope/pkg`.
74+
function packageName(specifier: string): string {
75+
const [first = '', second] = specifier.split('/');
76+
return first.startsWith('@') && second ? `${first}/${second}` : first;
77+
}
78+
79+
// Uses the last `node_modules` segment, so nested and pnpm store paths work.
80+
function ownerPackage(file: string): { name: string; dir: string } | undefined {
81+
const path = file.replace(/\\/g, '/');
82+
const start = path.lastIndexOf(NODE_MODULES);
83+
if (start === -1) {
84+
return undefined;
85+
}
86+
const name = packageName(path.slice(start + NODE_MODULES.length));
87+
return { name, dir: `${path.slice(0, start)}${NODE_MODULES}${name}` };
88+
}
89+
90+
// Matches like Vite. `external: true` is Vite's SSR default, not a choice for this package.
91+
function isUserExternal(external: readonly string[] | true, source: string, name: string): boolean {
92+
return Array.isArray(external) && external.some(entry => entry === source || entry === name);
93+
}
94+
95+
// These can be missing or native, like `pg-native`.
96+
function isOptionalDependency(packageDir: string, name: string): boolean {
97+
const manifest = readManifest(`${packageDir}/package.json`);
98+
if (!manifest) {
99+
return false;
100+
}
101+
const { peerDependenciesMeta, optionalDependencies } = manifest;
102+
const peerMeta = isPlainObject(peerDependenciesMeta) ? peerDependenciesMeta[name] : undefined;
103+
return (
104+
(isPlainObject(peerMeta) && peerMeta.optional === true) ||
105+
(isPlainObject(optionalDependencies) && name in optionalDependencies)
106+
);
107+
}
108+
109+
// The file that `require()` loads at runtime.
110+
function resolveWithNode(source: string, importer: string): string | undefined {
111+
try {
112+
return createRequire(importer).resolve(source);
113+
} catch {
114+
return undefined;
115+
}
116+
}
117+
118+
function readManifest(file: string): Record<string, unknown> | undefined {
119+
try {
120+
const manifest: unknown = JSON.parse(readFileSync(file, 'utf8'));
121+
return isPlainObject(manifest) ? manifest : undefined;
122+
} catch {
123+
return undefined;
124+
}
125+
}

‎packages/server-utils/src/orchestrion/bundler/vite.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ export type { Plugin as VitePlugin } from 'vite';
55
import { instrumentedModuleNames } from '../config';
66
import type { PluginOptions } from './options';
77
import { externalEntryMatchesModule, externalizedModulesWarning, orchestrionTransformOptions } from './options';
8+
import { createExternalRequireResolver } from './externalRequire';
89
import { resolveOrchestrionRuntimeRequest, SNIPPET_IMPORT_SPECIFIER } from './resolve';
910

1011
type TransformHandler = (this: unknown, code: string, id: string, opts?: { ssr?: boolean }) => unknown;
@@ -73,6 +74,7 @@ export function sentryOrchestrionPlugin(options: PluginOptions = {}): Plugin {
7374
...instrumentedModuleNames(options.instrumentations),
7475
'@sentry/server-utils',
7576
];
77+
const resolveExternalRequire = createExternalRequireResolver(noExternalModules());
7678

7779
const instance = {};
7880
// `true` when an earlier instance of this plugin is in the same build, which then transforms the modules alone.
@@ -94,7 +96,10 @@ export function sentryOrchestrionPlugin(options: PluginOptions = {}): Plugin {
9496
// packages into default imports, and `@sentry/core` has no default export, so the server
9597
// crashes at startup. Vite 8 keeps the `require()`s, which loads a second copy of `@sentry/core`.
9698
async resolveId(source, importer, resolveOptions) {
97-
if (source !== SNIPPET_IMPORT_SPECIFIER || !resolveOptions?.ssr) {
99+
if (source !== SNIPPET_IMPORT_SPECIFIER) {
100+
return resolveExternalRequire.call(this, source, importer, resolveOptions);
101+
}
102+
if (!resolveOptions?.ssr) {
98103
return null;
99104
}
100105
const resolved = await this.resolve(source, importer, { ...resolveOptions, skipSelf: true });

‎packages/server-utils/test/orchestrion/bundler.test.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -446,6 +446,27 @@ describe('sentryOrchestrionPlugin (vite)', () => {
446446
expect(fallback).toMatch(/[/\\]build[/\\]esm[/\\]index\.js$/);
447447
expect(existsSync(fallback as string)).toBe(true);
448448
});
449+
450+
it('resolves the snippet require() to the ESM entry before it handles other requires on Rolldown', async () => {
451+
const resolveId = vitePlugin().resolveId as (
452+
this: unknown,
453+
source: string,
454+
importer: string,
455+
opts: { ssr: boolean; kind: string },
456+
) => Promise<unknown>;
457+
const context = {
458+
meta: { rolldownVersion: '1.2.14' },
459+
environment: { name: 'ssr', mode: 'build', config: { resolve: { external: [] } } },
460+
resolve: vi.fn().mockResolvedValue({ id: '/app/node_modules/@sentry/server-utils/build/cjs/index.js' }),
461+
};
462+
463+
const resolved = await resolveId.call(context, '@sentry/server-utils', '/app/node_modules/pg/lib/client.js', {
464+
ssr: true,
465+
kind: 'require-call',
466+
});
467+
468+
expect(resolved).toEqual({ id: '/app/node_modules/@sentry/server-utils/build/esm/index.js' });
469+
});
449470
});
450471

451472
describe('sentryOrchestrionPlugin (bun)', () => {
Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,173 @@
1+
import { mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from 'node:fs';
2+
import { tmpdir } from 'node:os';
3+
import { join } from 'node:path';
4+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
5+
import { createExternalRequireResolver } from '../../src/orchestrion/bundler/externalRequire';
6+
7+
interface ContextOptions {
8+
environment?: string;
9+
meta?: Record<string, string>;
10+
mode?: string;
11+
external?: string[] | true;
12+
resolved?: { id: string; external?: boolean } | null;
13+
}
14+
15+
describe('createExternalRequireResolver', () => {
16+
let root: string;
17+
let pgFile: string;
18+
19+
function installPackage(name: string, manifest: Record<string, unknown> = {}, parentDir = root): string {
20+
const dir = join(parentDir, 'node_modules', name);
21+
mkdirSync(dir, { recursive: true });
22+
writeFileSync(join(dir, 'package.json'), JSON.stringify({ name, main: 'index.js', ...manifest }));
23+
writeFileSync(join(dir, 'index.js'), '');
24+
return join(dir, 'index.js');
25+
}
26+
27+
// By default, a Rolldown build in which Vite keeps the required package external.
28+
function context({
29+
environment = 'ssr',
30+
meta = { rolldownVersion: '1.2.14' },
31+
mode = 'build',
32+
external = [],
33+
resolved = { id: 'external', external: true },
34+
}: ContextOptions = {}): never {
35+
return {
36+
meta,
37+
environment: { name: environment, mode, config: { resolve: { external } } },
38+
resolve: vi.fn().mockResolvedValue(resolved),
39+
} as never;
40+
}
41+
42+
function resolveRequire(
43+
resolver: ReturnType<typeof createExternalRequireResolver>,
44+
source: string,
45+
importer: string,
46+
contextOptions?: ContextOptions,
47+
kind = 'require-call',
48+
): Promise<string | null> {
49+
return resolver.call(context(contextOptions), source, importer, { kind, isEntry: false } as never);
50+
}
51+
52+
beforeEach(() => {
53+
root = realpathSync(mkdtempSync(join(tmpdir(), 'orch-external-require-')));
54+
pgFile = join(root, 'node_modules/pg/lib/client.js');
55+
installPackage('pg');
56+
});
57+
58+
afterEach(() => {
59+
rmSync(root, { recursive: true, force: true });
60+
});
61+
62+
it('bundles an external package that an instrumented package requires', async () => {
63+
const pgTypes = installPackage('pg-types');
64+
65+
await expect(resolveRequire(createExternalRequireResolver(['pg']), 'pg-types', pgFile)).resolves.toBe(pgTypes);
66+
});
67+
68+
it.each([
69+
['it bundled', { id: 'external', external: true }],
70+
['Vite bundles itself', { id: 'pg-types/index.js' }],
71+
])('follows the requires of a package that %s', async (_, resolved) => {
72+
const pgTypes = installPackage('pg-types');
73+
const postgresArray = installPackage('postgres-array');
74+
const resolver = createExternalRequireResolver(['pg']);
75+
76+
await resolveRequire(resolver, 'pg-types', pgFile, { resolved });
77+
78+
await expect(resolveRequire(resolver, 'postgres-array', pgTypes)).resolves.toBe(postgresArray);
79+
});
80+
81+
// Every require() here resolves to a real file, so only the filter can return `null`.
82+
it.each([
83+
['app code', 'pg-types', 'src/db.js'],
84+
['a package that orchestrion does not bundle', 'pg-types', 'node_modules/other/index.js'],
85+
['a builtin', 'node:net', 'node_modules/pg/lib/client.js'],
86+
['a relative path', './connection', 'node_modules/pg/lib/client.js'],
87+
])('ignores a require() of %s', async (_, source, importer) => {
88+
installPackage('pg-types');
89+
installPackage('other');
90+
mkdirSync(join(root, 'node_modules/pg/lib'), { recursive: true });
91+
writeFileSync(join(root, 'node_modules/pg/lib/connection.js'), '');
92+
93+
await expect(
94+
resolveRequire(createExternalRequireResolver(['pg']), source, join(root, importer)),
95+
).resolves.toBeNull();
96+
});
97+
98+
it('bundles a require() from a pnpm store path', async () => {
99+
const store = join(root, 'node_modules/.pnpm/pg@8.23.1');
100+
installPackage('pg', {}, store);
101+
const pgTypes = installPackage('pg-types', {}, store);
102+
103+
await expect(
104+
resolveRequire(createExternalRequireResolver(['pg']), 'pg-types', join(store, 'node_modules/pg/lib/client.js')),
105+
).resolves.toBe(pgTypes);
106+
});
107+
108+
it('tracks bundled packages for each environment separately', async () => {
109+
const pgTypes = installPackage('pg-types');
110+
installPackage('postgres-array');
111+
const resolver = createExternalRequireResolver(['pg']);
112+
113+
await resolveRequire(resolver, 'pg-types', pgFile, { environment: 'ssr' });
114+
115+
await expect(resolveRequire(resolver, 'postgres-array', pgTypes, { environment: 'nitro' })).resolves.toBeNull();
116+
});
117+
118+
it.each([
119+
['on Rollup', { meta: { rollupVersion: '4.0.0' } }, 'require-call'],
120+
['on the dev server', { mode: 'dev' }, 'require-call'],
121+
['for an import', {}, 'import-statement'],
122+
])('leaves resolution to the bundler %s', async (_, contextOptions, kind) => {
123+
installPackage('pg-types');
124+
125+
await expect(
126+
resolveRequire(createExternalRequireResolver(['pg']), 'pg-types', pgFile, contextOptions, kind),
127+
).resolves.toBeNull();
128+
});
129+
130+
it.each([
131+
['an optional peer', { peerDependenciesMeta: { 'pg-native': { optional: true } } }],
132+
['an optional dependency', { optionalDependencies: { 'pg-native': '^3' } }],
133+
])('keeps %s external', async (_, manifest) => {
134+
installPackage('pg', manifest);
135+
installPackage('pg-native');
136+
137+
await expect(resolveRequire(createExternalRequireResolver(['pg']), 'pg-native', pgFile)).resolves.toBeNull();
138+
});
139+
140+
it('keeps a scoped subpath external when the user lists its package', async () => {
141+
installPackage('@scope/pkg');
142+
writeFileSync(join(root, 'node_modules/@scope/pkg/sub.js'), '');
143+
144+
await expect(
145+
resolveRequire(createExternalRequireResolver(['pg']), '@scope/pkg/sub', pgFile, { external: ['@scope/pkg'] }),
146+
).resolves.toBeNull();
147+
});
148+
149+
it('keeps a package external that the user lists in `external`', async () => {
150+
installPackage('pg-types');
151+
152+
await expect(
153+
resolveRequire(createExternalRequireResolver(['pg']), 'pg-types', pgFile, { external: ['pg-types'] }),
154+
).resolves.toBeNull();
155+
});
156+
157+
it.each([
158+
['only a subpath of the package', ['pg-types/lib/arrays']],
159+
['`true`, the SSR default', true],
160+
] as const)('bundles a package despite an external entry that names %s', async (_, external) => {
161+
const pgTypes = installPackage('pg-types');
162+
163+
await expect(
164+
resolveRequire(createExternalRequireResolver(['pg']), 'pg-types', pgFile, {
165+
external: external === true ? true : [...external],
166+
}),
167+
).resolves.toBe(pgTypes);
168+
});
169+
170+
it('keeps a package external that is not installed', async () => {
171+
await expect(resolveRequire(createExternalRequireResolver(['pg']), 'pg-types', pgFile)).resolves.toBeNull();
172+
});
173+
});

0 commit comments

Comments
 (0)