From 6d44952d4daaaab8ed9ec82212d2256c058eb05d Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Thu, 31 Oct 2024 10:02:31 -0700 Subject: [PATCH] fix(ext/node): resolve exports even if parent module filename isn't present (#26553) Fixes https://github.com/denoland/deno/issues/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. --- ext/node/ops/require.rs | 8 ++++++-- ext/node/polyfills/01_require.js | 6 +----- .../cjs-multiple-exports/1.0.0/package.json | 8 ++++++++ .../cjs-multiple-exports/1.0.0/src/add.js | 3 +++ .../cjs-multiple-exports/1.0.0/src/index.js | 3 +++ .../__test__.jsonc | 13 +++++++++++++ .../main.cjs | 16 ++++++++++++++++ .../package.json | 5 +++++ 8 files changed, 55 insertions(+), 7 deletions(-) create mode 100644 tests/registry/npm/@denotest/cjs-multiple-exports/1.0.0/package.json create mode 100644 tests/registry/npm/@denotest/cjs-multiple-exports/1.0.0/src/add.js create mode 100644 tests/registry/npm/@denotest/cjs-multiple-exports/1.0.0/src/index.js create mode 100644 tests/specs/node/require_export_from_parent_with_no_filename/__test__.jsonc create mode 100644 tests/specs/node/require_export_from_parent_with_no_filename/main.cjs create mode 100644 tests/specs/node/require_export_from_parent_with_no_filename/package.json diff --git a/ext/node/ops/require.rs b/ext/node/ops/require.rs index 43867053b0..7524fb43c7 100644 --- 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 index 5b0980c310..7935903a8d 100644 --- 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 new file mode 100644 index 0000000000..43f07a2351 --- /dev/null +++ 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" + } +} \ No newline at end of file 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 new file mode 100644 index 0000000000..42c8a7c604 --- /dev/null +++ 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; +}; \ No newline at end of file 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 new file mode 100644 index 0000000000..432ed652ed --- /dev/null +++ b/tests/registry/npm/@denotest/cjs-multiple-exports/1.0.0/src/index.js @@ -0,0 +1,3 @@ +module.exports = { + hello: "world" +}; \ No newline at end of file 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 new file mode 100644 index 0000000000..a3de08e460 --- /dev/null +++ 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 new file mode 100644 index 0000000000..3335ae1bf9 --- /dev/null +++ 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 new file mode 100644 index 0000000000..9cd6438953 --- /dev/null +++ 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" + } +}