Skip to content

Commit 74409c2

Browse files
sdkennedy2claude
andcommitted
fix(apps): parse backend module source instead of reading ModuleInfo#ast
The Apps backend module-graph collector read `moduleInfo.ast` in `moduleParsed`, coupling it to whatever AST the active bundler exposes. Rolldown — the bundler Vite 8 uses by default — deliberately stubs that getter to throw `UNSUPPORTED: ModuleInfo#ast`, so any app with a `.backend.ts` built under Vite 8 (or `rolldown-vite` on Vite 7) failed in `closeBundle` with an opaque error, after the client bundle had already succeeded. Parse `moduleInfo.code` with the plugin context's own `parse` instead. A bundler then only has to supply the module's source and its resolved dependency IDs, neither of which Rolldown withholds. Using the context's parser rather than bundling one keeps this correct by construction: the bundler already parsed this exact source to compute `importedIds`, so whatever it accepted, its parser accepts here. A third-party parser could disagree with the bundler; its own cannot. Verified `this.parse` is available in `moduleParsed` under both Rollup and Rolldown 1.1.5. It also means no TypeScript-capable parser is needed. `moduleParsed` runs after `transform`, so types and JSX are already gone — verified against a real Vite 8.1.5 + Rolldown 1.1.5 build, where `'x' as string` arrives as `"x"` and `<div/>` arrives as a `_jsx(...)` call. Two alternatives were measured and rejected: - Bundling `@typescript-eslint/typescript-estree`. It eagerly requires `typescript`, which it declares only as an optional peer, so it needed a deferred require to avoid dragging `typescript` into every consumer of every published plugin — and it added a dependency to all five of them for capability this path never exercises. - Injecting Vite's `parseAst` export. Vite maps its `require` condition to a reduced CJS entry that omits `parseAst`, so it is unavailable to consumers of the plugin's CJS build. Net dependency footprint is zero: no lockfile change and no change to any published package. Also filters before parsing, so `node_modules` files never reach the parser, and skips modules whose `code` is absent rather than handing a non-string to it. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 9797d6e commit 74409c2

6 files changed

Lines changed: 267 additions & 53 deletions

File tree

packages/plugins/apps/src/backend/ast-parsing/type-guards.ts

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,17 +13,28 @@ interface TypeScriptImportExportMetadata {
1313

1414
type TypeOnlyAwareNode = BaseNode & TypeScriptImportExportMetadata;
1515

16-
export function ensureProgram(node: BaseNode, filePath: string): Program {
16+
/**
17+
* Accepts `unknown` rather than `BaseNode` so it can narrow the output of a
18+
* bundler's `parse`, whose node types are declared separately from `estree` even
19+
* though they are structurally the same. The runtime check establishes the
20+
* shape, so callers get a validated `estree.Program` without a type assertion.
21+
*/
22+
export function ensureProgram(node: unknown, filePath: string): Program {
1723
if (!isProgramNode(node)) {
18-
throw new Error(
19-
`Expected a Program node from this.parse() for ${filePath}, got ${node.type}`,
20-
);
24+
throw new Error(`Expected a Program node for ${filePath}, got ${describeNodeType(node)}`);
2125
}
2226
return node;
2327
}
2428

25-
export function isProgramNode(node: BaseNode): node is Program {
26-
return node.type === 'Program';
29+
export function isProgramNode(node: unknown): node is Program {
30+
return typeof node === 'object' && node !== null && 'type' in node && node.type === 'Program';
31+
}
32+
33+
function describeNodeType(node: unknown): string {
34+
if (typeof node === 'object' && node !== null && 'type' in node) {
35+
return String(node.type);
36+
}
37+
return String(node);
2738
}
2839

2940
export function isStringLiteral(node: unknown): node is StringLiteral {

packages/plugins/apps/src/index.test.ts

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -40,18 +40,23 @@ function extractViteTransform(plugins: PluginOptions[]) {
4040
return (transform as { handler: (code: string, id: string) => Promise<unknown> }).handler;
4141
}
4242

43+
type ModuleParsedHook = (this: { parse: typeof parseAst }, moduleInfo: unknown) => void;
44+
4345
function emitModuleParsed(
44-
config: { plugins?: Array<{ moduleParsed?: (moduleInfo: unknown) => void }> },
46+
config: { plugins?: Array<{ moduleParsed?: ModuleParsedHook }> },
4547
id: string,
4648
code: string,
4749
importedIds: string[] = [],
4850
) {
4951
for (const plugin of config.plugins ?? []) {
50-
plugin.moduleParsed?.({
51-
id,
52-
ast: parseAst(code),
53-
importedIds,
54-
});
52+
plugin.moduleParsed?.call(
53+
{ parse: parseAst },
54+
{
55+
id,
56+
code,
57+
importedIds,
58+
},
59+
);
5560
}
5661
}
5762

packages/plugins/apps/src/vite/backend-module-graph-collector.test.ts

Lines changed: 171 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -6,40 +6,72 @@ import { parseAst } from 'rollup/parseAst';
66

77
import { createBackendModuleGraphCollector } from './backend-module-graph-collector';
88

9+
type FakeModuleInfo = {
10+
id: string;
11+
code?: string | null;
12+
importedIds?: string[];
13+
importedIdResolutions?: { id: string }[];
14+
};
15+
16+
const BUILD_ROOT = '/project';
17+
18+
const getModuleParsed = (buildRoot = BUILD_ROOT) => {
19+
const collector = createBackendModuleGraphCollector(buildRoot);
20+
const hook = collector.plugin.moduleParsed;
21+
if (typeof hook !== 'function') {
22+
throw new Error('Expected "moduleParsed" to be a function hook.');
23+
}
24+
25+
// Fills defaults in place rather than spreading into a new object: a spread
26+
// would enumerate every property, which reads the `ast` getter that some of
27+
// these cases deliberately make throw.
28+
const emit = (moduleInfo: FakeModuleInfo) => {
29+
const importedIds = moduleInfo.importedIds ?? [];
30+
moduleInfo.importedIds = importedIds;
31+
if (!('importedIdResolutions' in moduleInfo)) {
32+
moduleInfo.importedIdResolutions = importedIds.map((id) => ({ id }));
33+
}
34+
// Called with a plugin context exposing `parse`, which is where the hook
35+
// gets its parser. `rollup/parseAst` is what Rollup's real context uses,
36+
// so it is also the realistic stand-in here.
37+
// The hook only ever reads the handful of `ModuleInfo` fields modelled
38+
// by `FakeModuleInfo`; building a complete `ModuleInfo` would be noise.
39+
Reflect.apply(hook, { parse: parseAst }, [moduleInfo]);
40+
};
41+
42+
return { collector, emit };
43+
};
44+
45+
/**
46+
* Rolldown — the bundler Vite 8 uses by default — keeps `ModuleInfo#ast` on its
47+
* Rollup-compat object but stubs the getter to throw. Reading the property at
48+
* all is the failure, so the getter must throw rather than return undefined.
49+
*/
50+
const withUnsupportedAst = (moduleInfo: FakeModuleInfo): FakeModuleInfo =>
51+
Object.defineProperty(moduleInfo, 'ast', {
52+
get() {
53+
throw new Error('UNSUPPORTED: ModuleInfo#ast');
54+
},
55+
enumerable: true,
56+
});
57+
958
describe('Backend Functions - backend module graph collector', () => {
10-
test('Should collect parsed local module records from Rollup moduleParsed hooks', () => {
11-
const collector = createBackendModuleGraphCollector('/project');
12-
const moduleParsed = collector.plugin.moduleParsed as (moduleInfo: unknown) => void;
59+
test('Should collect parsed local module records from moduleParsed hooks', () => {
60+
const { collector, emit } = getModuleParsed();
1361

14-
moduleParsed({
62+
emit({
1563
id: '/project/src/backend/actions.backend.js?import',
16-
ast: parseAst(`
64+
code: `
1765
import { getEcho } from './helpers/http.js';
1866
export function run() {
1967
return getEcho();
2068
}
21-
`),
69+
`,
2270
importedIds: ['/project/src/backend/helpers/http.js?import'],
23-
importedIdResolutions: [{ id: '/project/src/backend/helpers/http.js?import' }],
24-
});
25-
moduleParsed({
26-
id: '/project/node_modules/package/index.js',
27-
ast: parseAst('export const value = true;'),
28-
importedIds: [],
29-
importedIdResolutions: [],
30-
});
31-
moduleParsed({
32-
id: '\0virtual-helper.js',
33-
ast: parseAst('export const value = true;'),
34-
importedIds: [],
35-
importedIdResolutions: [],
36-
});
37-
moduleParsed({
38-
id: 'virtual:dd-backend-dev:example.js',
39-
ast: parseAst('export const value = true;'),
40-
importedIds: [],
41-
importedIdResolutions: [],
4271
});
72+
emit({ id: '/project/node_modules/package/index.js', code: 'export const value = true;' });
73+
emit({ id: '\0virtual-helper.js', code: 'export const value = true;' });
74+
emit({ id: 'virtual:dd-backend-dev:example.js', code: 'export const value = true;' });
4375

4476
expect([...collector.getModuleRecords().keys()]).toEqual([
4577
'/project/src/backend/actions.backend.js',
@@ -55,4 +87,118 @@ describe('Backend Functions - backend module graph collector', () => {
5587
],
5688
});
5789
});
90+
91+
test('Should collect records under a bundler that does not support ModuleInfo#ast', () => {
92+
const { collector, emit } = getModuleParsed();
93+
const moduleInfo = withUnsupportedAst({
94+
id: '/project/src/backend/actions.backend.ts',
95+
code: `
96+
import { getEcho } from './helpers/http';
97+
export function run() {
98+
return getEcho();
99+
}
100+
`,
101+
importedIds: ['/project/src/backend/helpers/http.ts'],
102+
});
103+
104+
expect(() => emit(moduleInfo)).not.toThrow();
105+
expect(
106+
collector.getModuleRecords().get('/project/src/backend/actions.backend.ts'),
107+
).toMatchObject({
108+
staticDependencies: [
109+
{ source: './helpers/http', resolvedId: '/project/src/backend/helpers/http.ts' },
110+
],
111+
});
112+
});
113+
114+
test('Should never read ModuleInfo#ast, even for a module it skips', () => {
115+
const { emit } = getModuleParsed();
116+
const readAst = jest.fn(() => ({ type: 'Program', body: [] }));
117+
const moduleInfo: FakeModuleInfo = Object.defineProperty(
118+
{ id: '/project/src/backend/actions.backend.ts', code: 'export const a = 1;' },
119+
'ast',
120+
{ get: readAst, enumerable: true },
121+
);
122+
123+
emit(moduleInfo);
124+
emit(
125+
withUnsupportedAst({ id: '/project/node_modules/pkg/index.js', code: 'const a = 1;' }),
126+
);
127+
128+
expect(readAst).not.toHaveBeenCalled();
129+
});
130+
131+
describe('modules it must skip without parsing', () => {
132+
// Each of these would throw if the collector parsed it, so reaching the
133+
// parser at all is what the assertion catches.
134+
const cases = [
135+
{
136+
description: 'skip a package module rather than parse it',
137+
moduleInfo: {
138+
id: '/project/node_modules/package/index.js',
139+
code: 'this is not valid javascript {{{',
140+
},
141+
},
142+
{
143+
description: 'skip a module outside the build root',
144+
moduleInfo: { id: '/elsewhere/src/thing.ts', code: 'still ((( not valid' },
145+
},
146+
{
147+
description: 'skip a non-source extension',
148+
moduleInfo: { id: '/project/src/styles.css', code: '.a { color: red }' },
149+
},
150+
{
151+
description: 'skip a module the bundler reports with null code',
152+
moduleInfo: { id: '/project/src/external.ts', code: null },
153+
},
154+
{
155+
description: 'skip a module the bundler reports with no code at all',
156+
moduleInfo: { id: '/project/src/undefined.ts' },
157+
},
158+
];
159+
160+
test.each(cases)('Should $description', ({ moduleInfo }) => {
161+
const { collector, emit } = getModuleParsed();
162+
163+
expect(() => emit(moduleInfo)).not.toThrow();
164+
expect([...collector.getModuleRecords().keys()]).toEqual([]);
165+
});
166+
});
167+
168+
test('Should strip query suffixes from both the module and its dependencies', () => {
169+
const { collector, emit } = getModuleParsed();
170+
171+
emit({
172+
id: '/project/src/entry.backend.ts?v=abc123',
173+
code: `
174+
import { a } from './dep';
175+
export const run = () => a;
176+
`,
177+
importedIds: ['/project/src/dep.ts?v=def456'],
178+
});
179+
180+
expect([...collector.getModuleRecords().keys()]).toEqual(['/project/src/entry.backend.ts']);
181+
expect(
182+
collector.getModuleRecords().get('/project/src/entry.backend.ts')?.staticDependencies,
183+
).toEqual([{ source: './dep', resolvedId: '/project/src/dep.ts' }]);
184+
});
185+
186+
test('Should fall back to importedIds when the bundler omits importedIdResolutions', () => {
187+
const { collector, emit } = getModuleParsed();
188+
const id = '/project/src/entry.backend.ts';
189+
190+
emit({
191+
id,
192+
code: `
193+
import { a } from './dep';
194+
export const run = () => a;
195+
`,
196+
importedIds: ['/project/src/dep.ts'],
197+
importedIdResolutions: undefined,
198+
});
199+
200+
expect(collector.getModuleRecords().get(id)?.staticDependencies).toEqual([
201+
{ source: './dep', resolvedId: '/project/src/dep.ts' },
202+
]);
203+
});
58204
});

packages/plugins/apps/src/vite/backend-module-graph-collector.ts

Lines changed: 46 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,27 @@
22
// This product includes software developed at Datadog (https://www.datadoghq.com/).
33
// Copyright 2019-Present Datadog, Inc.
44

5-
import type { BaseNode } from 'estree';
65
import type { ModuleInfo } from 'rollup';
76
import type { Plugin } from 'vite';
87

98
import {
109
createParsedModuleRecord,
1110
type ParsedModuleRecord,
11+
shouldTraverseCollectedModule,
1212
} from '../backend/ast-parsing/module-graph';
13+
import { ensureProgram } from '../backend/ast-parsing/type-guards';
1314

1415
const VIRTUAL_MODULE_ID_RE = /^(?:\0|virtual:)/;
1516

17+
/**
18+
* The slice of the plugin context this hook uses. Declared structurally, rather
19+
* than using Rollup's `PluginContext`, so the same hook body type-checks against
20+
* Rollup's and Rolldown's separately-declared context types.
21+
*/
22+
interface ModuleParsedContext {
23+
parse: (code: string) => unknown;
24+
}
25+
1626
export interface BackendModuleGraphCollector {
1727
plugin: Plugin;
1828
getModuleRecords: () => ReadonlyMap<string, ParsedModuleRecord>;
@@ -24,17 +34,49 @@ export function createBackendModuleGraphCollector(buildRoot: string): BackendMod
2434
return {
2535
plugin: {
2636
name: 'dd-backend-module-graph-collector',
27-
moduleParsed(moduleInfo: ModuleInfo) {
37+
moduleParsed(this: ModuleParsedContext, moduleInfo: ModuleInfo) {
2838
const moduleId = normalizeViteModuleId(moduleInfo.id);
2939
if (isViteVirtualModuleId(moduleId)) {
3040
return;
3141
}
3242

43+
// Filter before parsing. `createParsedModuleRecord` applies the
44+
// same predicate, but reaching it means we already parsed the
45+
// module — which would put every `node_modules` file through the
46+
// parser and let an irrelevant syntax error fail the build.
47+
if (!shouldTraverseCollectedModule(moduleId, buildRoot)) {
48+
return;
49+
}
50+
51+
// Bundlers report `code: null` for external and synthetic
52+
// modules, and there is nothing to parse for those. Checked as a
53+
// string rather than against `null` so a bundler that leaves the
54+
// field undefined is handled the same way instead of reaching
55+
// the parser.
56+
const code = moduleInfo.code;
57+
if (typeof code !== 'string') {
58+
return;
59+
}
60+
61+
// Parse the source rather than reading `moduleInfo.ast`, which
62+
// Rolldown — the bundler Vite 8 uses by default — stubs to throw
63+
// `UNSUPPORTED: ModuleInfo#ast`.
64+
//
65+
// Using the context's own `parse` makes this correct by
66+
// construction: the bundler already parsed this exact source to
67+
// compute `importedIds`, so whatever it accepted, its parser
68+
// accepts here. It also means no TypeScript-capable parser is
69+
// needed — `moduleParsed` runs after `transform`, so types and
70+
// JSX are already gone.
71+
const parsed = this.parse(code);
72+
const ast = ensureProgram(parsed, moduleId);
73+
const staticDependencyIds =
74+
getStaticDependencyIds(moduleInfo).map(normalizeViteModuleId);
3375
const record = createParsedModuleRecord(
3476
moduleId,
3577
buildRoot,
36-
moduleInfo.ast as BaseNode,
37-
getStaticDependencyIds(moduleInfo).map(normalizeViteModuleId),
78+
ast,
79+
staticDependencyIds,
3880
);
3981
if (!record) {
4082
return;

packages/plugins/apps/src/vite/dev-server.test.ts

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -112,18 +112,23 @@ function mockBuildResult(code: string) {
112112
};
113113
}
114114

115+
type ModuleParsedHook = (this: { parse: typeof parseAst }, moduleInfo: unknown) => void;
116+
115117
function emitModuleParsed(
116-
config: { plugins?: Array<{ moduleParsed?: (moduleInfo: unknown) => void }> },
118+
config: { plugins?: Array<{ moduleParsed?: ModuleParsedHook }> },
117119
id: string,
118120
code: string,
119121
importedIds: string[] = [],
120122
) {
121123
for (const plugin of config.plugins ?? []) {
122-
plugin.moduleParsed?.({
123-
id,
124-
ast: parseAst(code),
125-
importedIds,
126-
});
124+
plugin.moduleParsed?.call(
125+
{ parse: parseAst },
126+
{
127+
id,
128+
code,
129+
importedIds,
130+
},
131+
);
127132
}
128133
}
129134

0 commit comments

Comments
 (0)