fix: resolve npm modules correctly (#656)

cloudflare · Mar 21, 2022 · aeb0fe0 · aeb0fe0
1 parent e3a458e
commit aeb0fe0
Show file tree

Hide file tree

Showing 3 changed files with 91 additions and 53 deletions.
diff --git a/.changeset/thick-bees-yell.md 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
@@ -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
@@ -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) => {