Skip to content

Commit 813aa58

Browse files
committed
fix(cli,build): scanner and suppression halves of the previous commit
Completes the change described in the prior commit message: the lexer, binding, cross-module and caching work in the scanner, the corrected suppression sources, and the never-throw additionalPackages declaration.
1 parent 6d68c53 commit 813aa58

5 files changed

Lines changed: 681 additions & 172 deletions

File tree

packages/build/src/extensions/core/additionalPackages.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,17 @@ export function additionalPackages(options: AdditionalPackagesOptions): BuildExt
2424
return [];
2525
}
2626

27-
return options.packages.map((pkg) => parsePackageName(pkg).name);
27+
const names: string[] = [];
28+
29+
for (const pkg of options.packages) {
30+
try {
31+
names.push(parsePackageName(pkg).name);
32+
} catch {
33+
continue;
34+
}
35+
}
36+
37+
return names;
2838
},
2939
async onBuildStart(context) {
3040
if (context.target !== "deploy") {

packages/cli-v3/src/build/buildWorker.ts

Lines changed: 14 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,15 @@
11
import { ResolvedConfig } from "@trigger.dev/core/v3/build";
22
import { BuildManifest, BuildTarget } from "@trigger.dev/core/v3/schemas";
3-
import * as esbuild from "esbuild";
43
import {
54
BundleResult,
65
bundleWorker,
76
createBuildManifestFromBundle,
87
logBuildWarnings,
98
} from "./bundle.js";
109
import {
10+
collectCreateRequireWarningMessages,
1111
CreateRequireCollector,
12-
createRequireUsageToWarning,
13-
unavailableCreateRequireUsages,
12+
extensionInstalledPackageMatchers,
1413
} from "./createRequireWarnings.js";
1514
import { bundleSkills } from "./bundleSkills.js";
1615
import {
@@ -19,7 +18,7 @@ import {
1918
notifyExtensionOnBuildStart,
2019
resolvePluginsForContext,
2120
} from "./extensions.js";
22-
import { createExternalsBuildExtension, deployExternalMatchers } from "./externals.js";
21+
import { createExternalsBuildExtension } from "./externals.js";
2322
import { tmpdir } from "node:os";
2423
import { mkdtemp, rm } from "node:fs/promises";
2524
import { join, relative, sep } from "node:path";
@@ -143,13 +142,17 @@ export async function buildWorker(options: BuildWorkerOptions) {
143142
buildManifest = await notifyExtensionOnBuildComplete(buildContext, buildManifest);
144143

145144
if (options.target !== "dev") {
146-
const buildWarnings = collectDeployBuildWarnings(
147-
bundleResult,
148-
createRequireCollector,
149-
buildManifest,
150-
resolvedConfig,
151-
options.forcedExternals
152-
);
145+
const buildWarnings = [
146+
...bundleResult.warnings.filter(
147+
(warning) => warning.location?.file && !warning.location.file.includes("node_modules")
148+
),
149+
...collectCreateRequireWarningMessages({
150+
usages: createRequireCollector.usages,
151+
buildManifest,
152+
extensionPackages: extensionInstalledPackageMatchers(resolvedConfig),
153+
target: options.target,
154+
}),
155+
];
153156

154157
if (buildWarnings.length > 0) {
155158
logBuildWarnings(buildWarnings);
@@ -170,36 +173,6 @@ export async function buildWorker(options: BuildWorkerOptions) {
170173
return buildManifest;
171174
}
172175

173-
/**
174-
* Deploy-only diagnostics: esbuild's own warnings scoped to the user's files,
175-
* plus packages loaded via createRequire() that end up neither bundled nor
176-
* installed in the image (not in the manifest's externals and not configured
177-
* as an external anywhere).
178-
*/
179-
function collectDeployBuildWarnings(
180-
bundleResult: BundleResult,
181-
createRequireCollector: CreateRequireCollector,
182-
buildManifest: BuildManifest,
183-
resolvedConfig: ResolvedConfig,
184-
forcedExternals: string[] = []
185-
): esbuild.PartialMessage[] {
186-
const esbuildWarnings = bundleResult.warnings.filter(
187-
(warning) => warning.location?.file && !warning.location.file.includes("node_modules")
188-
);
189-
190-
const installedPackages = new Set(
191-
(buildManifest.externals ?? []).map((external) => external.name)
192-
);
193-
194-
const createRequireWarnings = unavailableCreateRequireUsages(
195-
createRequireCollector.usages,
196-
installedPackages,
197-
deployExternalMatchers(resolvedConfig, forcedExternals)
198-
).map((usage) => createRequireUsageToWarning(usage, "deploy"));
199-
200-
return [...esbuildWarnings, ...createRequireWarnings];
201-
}
202-
203176
/** @knipignore Exported for the CLI end-to-end suite. */
204177
export function rewriteBuildManifestPaths(
205178
buildManifest: BuildManifest,

packages/cli-v3/src/build/createRequireWarnings.test.ts

Lines changed: 215 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,15 @@ import { mkdtemp, rm, writeFile } from "node:fs/promises";
33
import { tmpdir } from "node:os";
44
import { join } from "node:path";
55
import { describe, expect, it } from "vitest";
6+
import { ResolvedConfig } from "@trigger.dev/core/v3/build";
7+
import { BuildManifest } from "@trigger.dev/core/v3/schemas";
68
import {
9+
collectCreateRequireWarningMessages,
710
CreateRequireCollector,
811
createRequireUsageToWarning,
12+
extensionInstalledPackageMatchers,
913
packageNameForSpecifier,
14+
scanSource,
1015
scanSourceForCreateRequire,
1116
unavailableCreateRequireUsages,
1217
} from "./createRequireWarnings.js";
@@ -239,6 +244,71 @@ const mssql = createRequire(fileURLToPath(new URL(".", import.meta.url)))("mssql
239244
expect(scanSourceForCreateRequire(source).map((r) => r.specifier)).toEqual(["mssql"]);
240245
});
241246

247+
it("supports bindings from a dynamic import of the module builtin", () => {
248+
const source = `const { createRequire } = await import("node:module");
249+
const req = createRequire(import.meta.url);
250+
const pg = req("pg");
251+
`;
252+
253+
expect(scanSourceForCreateRequire(source).map((r) => r.specifier)).toEqual(["pg"]);
254+
});
255+
256+
it("supports a namespace bound from a dynamic import of the module builtin", () => {
257+
const source = `const mod = await import("node:module");
258+
const mssql = mod.createRequire(import.meta.url)("mssql");
259+
`;
260+
261+
expect(scanSourceForCreateRequire(source).map((r) => r.specifier)).toEqual(["mssql"]);
262+
});
263+
264+
it("supports declare-then-assign require variables", () => {
265+
const source = `import { createRequire } from "node:module";
266+
let req;
267+
req = createRequire(import.meta.url);
268+
const pg = req("pg");
269+
`;
270+
271+
expect(scanSourceForCreateRequire(source).map((r) => r.specifier)).toEqual(["pg"]);
272+
});
273+
274+
it("does not warn for call-shaped text inside string literals", () => {
275+
const source = `import { createRequire } from "node:module";
276+
const req = createRequire(import.meta.url);
277+
const msg = 'try req("mssql") for details';
278+
const pg = req("pg");
279+
`;
280+
281+
expect(scanSourceForCreateRequire(source).map((r) => r.specifier)).toEqual(["pg"]);
282+
});
283+
284+
it("strips a comment that follows a regex literal containing quotes", () => {
285+
const source = `import { createRequire } from "node:module";
286+
const req = createRequire(import.meta.url);
287+
const quote = /['"]/; /* old: req("bcrypt") */
288+
const pg = req("pg");
289+
`;
290+
291+
expect(scanSourceForCreateRequire(source).map((r) => r.specifier)).toEqual(["pg"]);
292+
});
293+
294+
it("follows require functions imported from other scanned files", () => {
295+
const util = `import { createRequire } from "node:module";
296+
export const cjsRequire = createRequire(import.meta.url);
297+
`;
298+
const task = `import { cjsRequire } from "./util.js";
299+
const mssql = cjsRequire("mssql");
300+
`;
301+
302+
const { exportedRequireFns, specifiers } = scanSource(util);
303+
304+
expect(exportedRequireFns).toEqual(["cjsRequire"]);
305+
expect(specifiers).toEqual([]);
306+
307+
const taskResults = scanSourceForCreateRequire(task, new Set(exportedRequireFns));
308+
309+
expect(taskResults.map((r) => r.specifier)).toEqual(["mssql"]);
310+
});
311+
242312
it("returns nothing when the source doesn't mention createRequire", () => {
243313
const source = `import mssql from "mssql";
244314
export const pool = mssql.connect();
@@ -312,6 +382,51 @@ export const mssql = createRequire(import.meta.url)("mssql");
312382
await rm(dir, { recursive: true, force: true });
313383
}
314384
});
385+
386+
it("collects usages of a require function imported from another module", async () => {
387+
const dir = await mkdtemp(join(tmpdir(), "create-require-collector-"));
388+
389+
try {
390+
await writeFile(
391+
join(dir, "util.ts"),
392+
`import { createRequire } from "node:module";
393+
export const cjsRequire = createRequire(import.meta.url);
394+
`
395+
);
396+
397+
const entryPoint = join(dir, "entry.ts");
398+
await writeFile(
399+
entryPoint,
400+
`import { cjsRequire } from "./util.js";
401+
export const mssql = cjsRequire("mssql");
402+
`
403+
);
404+
405+
const collector = new CreateRequireCollector(dir);
406+
407+
await build({
408+
entryPoints: [entryPoint],
409+
bundle: true,
410+
metafile: true,
411+
write: false,
412+
format: "esm",
413+
platform: "node",
414+
outdir: dir,
415+
absWorkingDir: dir,
416+
logLevel: "silent",
417+
plugins: [collector.plugin],
418+
});
419+
420+
expect(collector.usages).toHaveLength(1);
421+
expect(collector.usages[0]).toMatchObject({
422+
specifier: "mssql",
423+
packageName: "mssql",
424+
file: "entry.ts",
425+
});
426+
} finally {
427+
await rm(dir, { recursive: true, force: true });
428+
}
429+
});
315430
});
316431

317432
describe("createRequireUsageToWarning", () => {
@@ -378,6 +493,106 @@ describe("unavailableCreateRequireUsages", () => {
378493
});
379494
});
380495

496+
describe("extensionInstalledPackageMatchers", () => {
497+
const configWith = (extensions: unknown[]) =>
498+
({ build: { extensions } }) as unknown as ResolvedConfig;
499+
500+
it("collects matchers from installedPackagesForTarget and externalsForTarget", () => {
501+
const { matchers, incomplete } = extensionInstalledPackageMatchers(
502+
configWith([
503+
{ name: "custom", installedPackagesForTarget: () => ["ffmpeg-static"] },
504+
{ name: "prisma", externalsForTarget: () => ["@prisma/client"] },
505+
])
506+
);
507+
508+
expect(incomplete).toBe(false);
509+
expect(matchers.some((m) => m.test("ffmpeg-static"))).toBe(true);
510+
expect(matchers.some((m) => m.test("@prisma/client"))).toBe(true);
511+
expect(matchers.some((m) => m.test("mssql"))).toBe(false);
512+
});
513+
514+
it("marks the result incomplete instead of throwing when an extension hook throws", () => {
515+
const { incomplete } = extensionInstalledPackageMatchers(
516+
configWith([
517+
{
518+
name: "boom",
519+
installedPackagesForTarget: () => {
520+
throw new Error("bad package entry");
521+
},
522+
},
523+
])
524+
);
525+
526+
expect(incomplete).toBe(true);
527+
});
528+
529+
it("marks the result incomplete for an additionalPackages extension without the hook", () => {
530+
const { incomplete } = extensionInstalledPackageMatchers(
531+
configWith([{ name: "additionalPackages" }])
532+
);
533+
534+
expect(incomplete).toBe(true);
535+
});
536+
});
537+
538+
describe("collectCreateRequireWarningMessages", () => {
539+
const usage = {
540+
specifier: "mssql",
541+
packageName: "mssql",
542+
file: "src/db.ts",
543+
line: 1,
544+
column: 0,
545+
lineText: "",
546+
};
547+
548+
const manifestWith = (externals: Array<{ name: string; version: string }>) =>
549+
({ externals }) as unknown as BuildManifest;
550+
551+
it("warns for a package missing from the manifest externals", () => {
552+
const messages = collectCreateRequireWarningMessages({
553+
usages: [usage],
554+
buildManifest: manifestWith([]),
555+
extensionPackages: { matchers: [], incomplete: false },
556+
target: "deploy",
557+
});
558+
559+
expect(messages).toHaveLength(1);
560+
});
561+
562+
it("suppresses packages present in the manifest externals", () => {
563+
const messages = collectCreateRequireWarningMessages({
564+
usages: [usage],
565+
buildManifest: manifestWith([{ name: "mssql", version: "10.0.0" }]),
566+
extensionPackages: { matchers: [], incomplete: false },
567+
target: "deploy",
568+
});
569+
570+
expect(messages).toEqual([]);
571+
});
572+
573+
it("stays silent in dev when extension-installed packages are unknown", () => {
574+
const messages = collectCreateRequireWarningMessages({
575+
usages: [usage],
576+
buildManifest: manifestWith([]),
577+
extensionPackages: { matchers: [], incomplete: true },
578+
target: "dev",
579+
});
580+
581+
expect(messages).toEqual([]);
582+
});
583+
584+
it("still warns on deploy when extension-installed packages are unknown", () => {
585+
const messages = collectCreateRequireWarningMessages({
586+
usages: [usage],
587+
buildManifest: manifestWith([]),
588+
extensionPackages: { matchers: [], incomplete: true },
589+
target: "deploy",
590+
});
591+
592+
expect(messages).toHaveLength(1);
593+
});
594+
});
595+
381596
describe("packageNameForSpecifier", () => {
382597
it("extracts the package name from plain, subpath, and scoped specifiers", () => {
383598
expect(packageNameForSpecifier("mssql")).toBe("mssql");

0 commit comments

Comments
 (0)