From aeb0fe02dbc9b8ef2edc0e2a669315bd40bbdfb3 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Mon, 21 Mar 2022 20:28:37 +0530 Subject: [PATCH] fix: resolve npm modules correctly (#656) --- .changeset/thick-bees-yell.md | 9 ++ .../wrangler/src/__tests__/publish.test.ts | 24 ++++ packages/wrangler/src/module-collection.ts | 111 +++++++++--------- 3 files changed, 91 insertions(+), 53 deletions(-) create mode 100644 .changeset/thick-bees-yell.md diff --git a/.changeset/thick-bees-yell.md b/.changeset/thick-bees-yell.md new file mode 100644 index 000000000000..33311272f15b --- /dev/null +++ b/.changeset/thick-bees-yell.md @@ -0,0 +1,9 @@ +--- +"wrangler": patch +--- + +fix: resolve npm modules correctly + +When implementing legacy module specifiers, we didn't throughly test the interaction when there weren't any other files next to the entry worker, and importing npm modules. It would create a Regex that matched _every_ import, and fail because a file of that name wasn't present in the source directory. This fix constructs a better regex, applies it only when there are more files next to the worker, and increases test coverage for that scenario. + +Fixes https://github.com/cloudflare/wrangler2/issues/655 diff --git a/packages/wrangler/src/__tests__/publish.test.ts b/packages/wrangler/src/__tests__/publish.test.ts index b26eb58747b3..4f6b55162102 100644 --- a/packages/wrangler/src/__tests__/publish.test.ts +++ b/packages/wrangler/src/__tests__/publish.test.ts @@ -2362,6 +2362,30 @@ export default{ `"Deprecation warning: detected a legacy module import in \\"./index.js\\". This will stop working in the future. Replace references to \\"index.wasm\\" with \\"./index.wasm\\";"` ); }); + + it("should not match regular module specifiers when there aren't any possible legacy module matches", async () => { + // see https://github.com/cloudflare/wrangler2/issues/655 for bug details + + fs.writeFileSync( + "./index.js", + `import inner from './inner/index.js'; export default {};` + ); + fs.mkdirSync("./inner", { recursive: true }); + fs.writeFileSync("./inner/index.js", `export default 123`); + mockSubDomainRequest(); + mockUploadWorkerRequest(); + + await runWrangler( + "publish index.js --compatibility-date 2022-03-17 --name test-name" + ); + expect(std.out).toMatchInlineSnapshot(` + "Uploaded test-name (TIMINGS) + Published test-name (TIMINGS) + test-name.test-sub-domain.workers.dev" + `); + expect(std.err).toMatchInlineSnapshot(`""`); + expect(std.warn).toMatchInlineSnapshot(`""`); + }); }); /** Write a mock Worker script to disk. */ diff --git a/packages/wrangler/src/module-collection.ts b/packages/wrangler/src/module-collection.ts index 556ef03ce905..00fff432c593 100644 --- a/packages/wrangler/src/module-collection.ts +++ b/packages/wrangler/src/module-collection.ts @@ -117,62 +117,67 @@ export default function createModuleCollector(props: { }); }); - build.onResolve( - { - filter: new RegExp( - [...props.wrangler1xlegacyModuleReferences.fileNames] - .map((fileName) => `^${fileName}$`) - .join("|") - ), - }, - async (args: esbuild.OnResolveArgs) => { - if ( - args.kind !== "import-statement" && - args.kind !== "require-call" - ) { - return; - } - // In the future, this will simply throw an error - console.warn( - `Deprecation warning: detected a legacy module import in "./${path.relative( - process.cwd(), - args.importer - )}". This will stop working in the future. Replace references to "${ + if (props.wrangler1xlegacyModuleReferences.fileNames.size > 0) { + build.onResolve( + { + filter: new RegExp( + "^(" + + [...props.wrangler1xlegacyModuleReferences.fileNames].join( + "|" + ) + + ")$" + ), + }, + async (args: esbuild.OnResolveArgs) => { + if ( + args.kind !== "import-statement" && + args.kind !== "require-call" + ) { + return; + } + // In the future, this will simply throw an error + console.warn( + `Deprecation warning: detected a legacy module import in "./${path.relative( + process.cwd(), + args.importer + )}". This will stop working in the future. Replace references to "${ + args.path + }" with "./${args.path}";` + ); + + // take the file and massage it to a + // transportable/manageable format + const filePath = path.join( + props.wrangler1xlegacyModuleReferences.rootDirectory, args.path - }" with "./${args.path}";` - ); + ); + const fileContent = await readFile(filePath); + const fileHash = crypto + .createHash("sha1") + .update(fileContent) + .digest("hex"); + const fileName = `./${fileHash}-${path.basename(args.path)}`; - // take the file and massage it to a - // transportable/manageable format - const filePath = path.join( - props.wrangler1xlegacyModuleReferences.rootDirectory, - args.path - ); - const fileContent = await readFile(filePath); - const fileHash = crypto - .createHash("sha1") - .update(fileContent) - .digest("hex"); - const fileName = `./${fileHash}-${path.basename(args.path)}`; - - const { rule } = - rulesMatchers.find(({ regex }) => regex.test(fileName)) || {}; - if (rule) { - // add the module to the array - modules.push({ - name: fileName, - content: fileContent, - type: RuleTypeToModuleType[rule.type], - }); - return { - path: fileName, // change the reference to the changed module - external: props.format === "modules", // mark it as external in the bundle - namespace: `wrangler-module-${rule.type}`, // just a tag, this isn't strictly necessary - watchFiles: [filePath], // we also add the file to esbuild's watch list - }; + const { rule } = + rulesMatchers.find(({ regex }) => regex.test(fileName)) || {}; + if (rule) { + // add the module to the array + modules.push({ + name: fileName, + content: fileContent, + type: RuleTypeToModuleType[rule.type], + }); + return { + path: fileName, // change the reference to the changed module + external: props.format === "modules", // mark it as external in the bundle + namespace: `wrangler-module-${rule.type}`, // just a tag, this isn't strictly necessary + watchFiles: [filePath], // we also add the file to esbuild's watch list + }; + } } - } - ); + ); + } + // ~ end legacy module specifier support ~ rules?.forEach((rule) => {