fix(ext/node): resolve exports even if parent module filename isn't p…

…resent (#26553) Fixes #26505 I'm not exactly sure how this case comes about (I tried to write tests for it but couldn't manage to reproduce it), but what happens is the parent filename ends up null, and we bail out of resolving the specifier in package exports. I've checked, and in node the parent filename is also null (so that's not a bug on our part), but node continues to resolve even in that case. So this PR should match node's behavior more closely than we currently do.
denoland · Nov 5, 2024 · 68cee70 · 68cee70
1 parent d284d1c
commit 68cee70
Show file tree

Hide file tree

Showing 8 changed files with 55 additions and 7 deletions.
diff --git a/ext/node/ops/require.rs b/ext/node/ops/require.rs
@@ -529,12 +529,16 @@ where
     return Ok(None);
   };
 
-  let referrer = Url::from_file_path(parent_path).unwrap();
+  let referrer = if parent_path.is_empty() {
+    None
+  } else {
+    Some(Url::from_file_path(parent_path).unwrap())
+  };
   let r = node_resolver.package_exports_resolve(
     &pkg.path,
     &format!(".{expansion}"),
     exports,
-    Some(&referrer),
+    referrer.as_ref(),
     NodeModuleKind::Cjs,
     REQUIRE_CONDITIONS,
     NodeResolutionMode::Execution,

diff --git a/ext/node/polyfills/01_require.js b/ext/node/polyfills/01_require.js
@@ -523,17 +523,13 @@ function resolveExports(
     return;
   }
 
-  if (!parentPath) {
-    return false;
-  }
-
   return op_require_resolve_exports(
     usesLocalNodeModulesDir,
     modulesPath,
     request,
     name,
     expansion,
-    parentPath,
+    parentPath ?? "",
   ) ?? false;
 }
 

diff --git a/tests/registry/npm/@denotest/cjs-multiple-exports/1.0.0/package.json b/tests/registry/npm/@denotest/cjs-multiple-exports/1.0.0/package.json
@@ -0,0 +1,8 @@
+{
+  "name": "@denotest/cjs-multiple-exports",
+  "version": "1.0.0",
+  "exports": {
+    ".": "./src/index.js",
+    "./add": "./src/add.js"
+  }
+}
diff --git a/tests/registry/npm/@denotest/cjs-multiple-exports/1.0.0/src/add.js b/tests/registry/npm/@denotest/cjs-multiple-exports/1.0.0/src/add.js
@@ -0,0 +1,3 @@
+module.exports = function add(a, b) {
+  return a + b;
+};
diff --git a/tests/registry/npm/@denotest/cjs-multiple-exports/1.0.0/src/index.js b/tests/registry/npm/@denotest/cjs-multiple-exports/1.0.0/src/index.js
@@ -0,0 +1,3 @@
+module.exports = {
+  hello: "world"
+};
diff --git a/tests/specs/node/require_export_from_parent_with_no_filename/__test__.jsonc b/tests/specs/node/require_export_from_parent_with_no_filename/__test__.jsonc
@@ -0,0 +1,13 @@
+{
+  "tempDir": true,
+  "steps": [
+    {
+      "args": "install",
+      "output": "[WILDCARD]"
+    },
+    {
+      "args": "run -A main.cjs",
+      "output": "3\n"
+    }
+  ]
+}
diff --git a/tests/specs/node/require_export_from_parent_with_no_filename/main.cjs b/tests/specs/node/require_export_from_parent_with_no_filename/main.cjs
@@ -0,0 +1,16 @@
+const path = require("node:path");
+const Module = require("node:module");
+function requireFromString(code, filename) {
+  const paths = Module._nodeModulePaths((0, path.dirname)(filename));
+  const m = new Module(filename, module.parent);
+  m.paths = paths;
+  m._compile(code, filename);
+  return m.exports;
+}
+
+const code = `
+const add = require("@denotest/cjs-multiple-exports/add");
+
+console.log(add(1, 2));
+`;
+requireFromString(code, "fake.js");
diff --git a/tests/specs/node/require_export_from_parent_with_no_filename/package.json b/tests/specs/node/require_export_from_parent_with_no_filename/package.json
@@ -0,0 +1,5 @@
+{
+  "dependencies": {
+    "@denotest/cjs-multiple-exports": "1.0.0"
+  }
+}