From 5d3d4eba398ce862aa57d5de43d17e7ae1d45d3c Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sat, 9 Mar 2024 10:21:31 -0500 Subject: [PATCH] fix(node): require of pkg json imports was broken (#22821) --- ext/node/lib.rs | 3 + ext/node/ops/http2.rs | 1 - ext/node/ops/ipc.rs | 1 + ext/node/ops/require.rs | 124 ++++++++++-------- ext/node/resolution.rs | 33 +++-- tests/integration/npm_tests.rs | 7 + tests/testdata/npm/cjs_pkg_imports/main.out | 3 + tests/testdata/npm/cjs_pkg_imports/main.ts | 3 + .../@denotest/cjs-pkg-imports/1.0.0/index.js | 7 + .../@denotest/cjs-pkg-imports/1.0.0/number.js | 1 + .../cjs-pkg-imports/1.0.0/package.json | 13 ++ .../cjs-pkg-imports/1.0.0/sub/dist/crypto.js | 1 + .../cjs-pkg-imports/1.0.0/sub/dist/crypto.mjs | 1 + 13 files changed, 129 insertions(+), 69 deletions(-) create mode 100644 tests/testdata/npm/cjs_pkg_imports/main.out create mode 100644 tests/testdata/npm/cjs_pkg_imports/main.ts create mode 100644 tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/index.js create mode 100644 tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/number.js create mode 100644 tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/package.json create mode 100644 tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/sub/dist/crypto.js create mode 100644 tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/sub/dist/crypto.mjs diff --git a/ext/node/lib.rs b/ext/node/lib.rs index 40c9e70199..6d5a21acea 100644 --- a/ext/node/lib.rs +++ b/ext/node/lib.rs @@ -1,5 +1,8 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. +#![deny(clippy::print_stderr)] +#![deny(clippy::print_stdout)] + use std::collections::HashSet; use std::path::Path; use std::path::PathBuf; diff --git a/ext/node/ops/http2.rs b/ext/node/ops/http2.rs index 3579ade980..b89990e0ba 100644 --- a/ext/node/ops/http2.rs +++ b/ext/node/ops/http2.rs @@ -505,7 +505,6 @@ pub async fn op_http2_client_get_response_body_chunk( return Ok((Some(data.to_vec()), false)); } DataOrTrailers::Trailers(trailers) => { - println!("{trailers:?}"); if let Some(trailers_tx) = RcRef::map(&resource, |r| &r.trailers_tx) .borrow_mut() .await diff --git a/ext/node/ops/ipc.rs b/ext/node/ops/ipc.rs index 12e8407462..3cdb569abc 100644 --- a/ext/node/ops/ipc.rs +++ b/ext/node/ops/ipc.rs @@ -437,6 +437,7 @@ mod impl_ { (client, server) } + #[allow(clippy::print_stdout)] #[tokio::test] async fn bench_ipc() -> Result<(), Box> { // A simple round trip benchmark for quick dev feedback. diff --git a/ext/node/ops/require.rs b/ext/node/ops/require.rs index 995feb4a10..6ca0b4a1a1 100644 --- a/ext/node/ops/require.rs +++ b/ext/node/ops/require.rs @@ -97,17 +97,16 @@ where { let fs = state.borrow::(); // Guarantee that "from" is absolute. - let from = if from.starts_with("file:///") { - Url::parse(&from)?.to_file_path().unwrap() + let from_url = if from.starts_with("file:///") { + Url::parse(&from)? } else { deno_core::resolve_path( &from, &(fs.cwd().map_err(AnyError::from)).context("Unable to get CWD")?, ) .unwrap() - .to_file_path() - .unwrap() }; + let from = url_to_file_path(&from_url)?; ensure_read_permission::

(state, &from)?; @@ -420,24 +419,21 @@ where let referrer = deno_core::url::Url::from_file_path(&pkg.path).unwrap(); if let Some(exports) = &pkg.exports { - node_resolver - .package_exports_resolve( - &pkg.path, - &expansion, - exports, - &referrer, - NodeModuleKind::Cjs, - resolution::REQUIRE_CONDITIONS, - NodeResolutionMode::Execution, - permissions, - ) - .map(|r| { - Some(if r.scheme() == "file" { - r.to_file_path().unwrap().to_string_lossy().to_string() - } else { - r.to_string() - }) - }) + let r = node_resolver.package_exports_resolve( + &pkg.path, + &expansion, + exports, + &referrer, + NodeModuleKind::Cjs, + resolution::REQUIRE_CONDITIONS, + NodeResolutionMode::Execution, + permissions, + )?; + Ok(Some(if r.scheme() == "file" { + url_to_file_path_string(&r)? + } else { + r.to_string() + })) } else { Ok(None) } @@ -510,24 +506,21 @@ where if let Some(exports) = &pkg.exports { let referrer = Url::from_file_path(parent_path).unwrap(); - node_resolver - .package_exports_resolve( - &pkg.path, - &format!(".{expansion}"), - exports, - &referrer, - NodeModuleKind::Cjs, - resolution::REQUIRE_CONDITIONS, - NodeResolutionMode::Execution, - permissions, - ) - .map(|r| { - Some(if r.scheme() == "file" { - r.to_file_path().unwrap().to_string_lossy().to_string() - } else { - r.to_string() - }) - }) + let r = node_resolver.package_exports_resolve( + &pkg.path, + &format!(".{expansion}"), + exports, + &referrer, + NodeModuleKind::Cjs, + resolution::REQUIRE_CONDITIONS, + NodeResolutionMode::Execution, + permissions, + )?; + Ok(Some(if r.scheme() == "file" { + url_to_file_path_string(&r)? + } else { + r.to_string() + })) } else { Ok(None) } @@ -575,32 +568,35 @@ where #[string] pub fn op_require_package_imports_resolve

( state: &mut OpState, - #[string] parent_filename: String, + #[string] referrer_filename: String, #[string] request: String, ) -> Result, AnyError> where P: NodePermissions + 'static, { - let parent_path = PathBuf::from(&parent_filename); - ensure_read_permission::

(state, &parent_path)?; + let referrer_path = PathBuf::from(&referrer_filename); + ensure_read_permission::

(state, &referrer_path)?; let node_resolver = state.borrow::>(); let permissions = state.borrow::

(); - let pkg = node_resolver - .load_package_json(permissions, parent_path.join("package.json"))?; + let Some(pkg) = node_resolver + .get_closest_package_json_from_path(&referrer_path, permissions)? + else { + return Ok(None); + }; if pkg.imports.is_some() { - let referrer = - deno_core::url::Url::from_file_path(&parent_filename).unwrap(); - node_resolver - .package_imports_resolve( - &request, - &referrer, - NodeModuleKind::Cjs, - resolution::REQUIRE_CONDITIONS, - NodeResolutionMode::Execution, - permissions, - ) - .map(|r| Some(r.to_string())) + let referrer_url = + deno_core::url::Url::from_file_path(&referrer_filename).unwrap(); + let url = node_resolver.package_imports_resolve( + &request, + &referrer_url, + NodeModuleKind::Cjs, + Some(&pkg), + resolution::REQUIRE_CONDITIONS, + NodeResolutionMode::Execution, + permissions, + )?; + Ok(Some(url_to_file_path_string(&url)?)) } else { Ok(None) } @@ -613,3 +609,17 @@ pub fn op_require_break_on_next_statement(state: &mut OpState) { .borrow_mut() .wait_for_session_and_break_on_next_statement() } + +fn url_to_file_path_string(url: &Url) -> Result { + let file_path = url_to_file_path(url)?; + Ok(file_path.to_string_lossy().to_string()) +} + +fn url_to_file_path(url: &Url) -> Result { + match url.to_file_path() { + Ok(file_path) => Ok(file_path), + Err(()) => { + deno_core::anyhow::bail!("failed to convert '{}' to file path", url) + } + } +} diff --git a/ext/node/resolution.rs b/ext/node/resolution.rs index 4590311e8b..a1ee0e9ef7 100644 --- a/ext/node/resolution.rs +++ b/ext/node/resolution.rs @@ -239,10 +239,12 @@ impl NodeResolver { Some(resolved_specifier) } } else if specifier.starts_with('#') { + let pkg_config = self.get_closest_package_json(referrer, permissions)?; Some(self.package_imports_resolve( specifier, referrer, NodeModuleKind::Esm, + pkg_config.as_ref(), conditions, mode, permissions, @@ -525,11 +527,13 @@ impl NodeResolver { None } + #[allow(clippy::too_many_arguments)] pub(super) fn package_imports_resolve( &self, name: &str, referrer: &ModuleSpecifier, referrer_kind: NodeModuleKind, + referrer_pkg_json: Option<&PackageJson>, conditions: &[&str], mode: NodeResolutionMode, permissions: &dyn NodePermissions, @@ -544,12 +548,10 @@ impl NodeResolver { } let mut package_json_path = None; - if let Some(package_config) = - self.get_closest_package_json(referrer, permissions)? - { - if package_config.exists { - package_json_path = Some(package_config.path.clone()); - if let Some(imports) = &package_config.imports { + if let Some(pkg_json) = &referrer_pkg_json { + if pkg_json.exists { + package_json_path = Some(pkg_json.path.clone()); + if let Some(imports) = &pkg_json.imports { if imports.contains_key(name) && !name.contains('*') { let target = imports.get(name).unwrap(); let maybe_resolved = self.resolve_package_target( @@ -1121,7 +1123,19 @@ impl NodeResolver { url: &ModuleSpecifier, permissions: &dyn NodePermissions, ) -> Result, AnyError> { - let Some(package_json_path) = self.get_closest_package_json_path(url)? + let Ok(file_path) = url.to_file_path() else { + return Ok(None); + }; + self.get_closest_package_json_from_path(&file_path, permissions) + } + + pub fn get_closest_package_json_from_path( + &self, + file_path: &Path, + permissions: &dyn NodePermissions, + ) -> Result, AnyError> { + let Some(package_json_path) = + self.get_closest_package_json_path(file_path)? else { return Ok(None); }; @@ -1132,11 +1146,8 @@ impl NodeResolver { fn get_closest_package_json_path( &self, - url: &ModuleSpecifier, + file_path: &Path, ) -> Result, AnyError> { - let Ok(file_path) = url.to_file_path() else { - return Ok(None); - }; let current_dir = deno_core::strip_unc_prefix( self.fs.realpath_sync(file_path.parent().unwrap())?, ); diff --git a/tests/integration/npm_tests.rs b/tests/integration/npm_tests.rs index 7c34415dab..7df9e4f8ab 100644 --- a/tests/integration/npm_tests.rs +++ b/tests/integration/npm_tests.rs @@ -477,6 +477,13 @@ itest!(run_existing_npm_package_with_subpath { copy_temp_dir: Some("npm/run_existing_npm_package_with_subpath/"), }); +itest!(cjs_pkg_imports { + args: "run -A npm/cjs_pkg_imports/main.ts", + output: "npm/cjs_pkg_imports/main.out", + envs: env_vars_for_npm_tests(), + http_server: true, +}); + #[test] fn parallel_downloading() { let (out, _err) = util::run_and_collect_output_with_args( diff --git a/tests/testdata/npm/cjs_pkg_imports/main.out b/tests/testdata/npm/cjs_pkg_imports/main.out new file mode 100644 index 0000000000..b2df56f802 --- /dev/null +++ b/tests/testdata/npm/cjs_pkg_imports/main.out @@ -0,0 +1,3 @@ +Download http://localhost:4545/npm/registry/@denotest/cjs-pkg-imports +Download http://localhost:4545/npm/registry/@denotest/cjs-pkg-imports/1.0.0.tgz +{ crypto: Crypto { subtle: SubtleCrypto {} }, number: 5 } diff --git a/tests/testdata/npm/cjs_pkg_imports/main.ts b/tests/testdata/npm/cjs_pkg_imports/main.ts new file mode 100644 index 0000000000..b30a3f85cd --- /dev/null +++ b/tests/testdata/npm/cjs_pkg_imports/main.ts @@ -0,0 +1,3 @@ +import crypto from "npm:@denotest/cjs-pkg-imports"; + +console.log(crypto); diff --git a/tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/index.js b/tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/index.js new file mode 100644 index 0000000000..0f86652770 --- /dev/null +++ b/tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/index.js @@ -0,0 +1,7 @@ +const crypto = require("#crypto"); +const number = require("#number"); + +module.exports = { + crypto, + number, +}; diff --git a/tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/number.js b/tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/number.js new file mode 100644 index 0000000000..f4e8d9d29a --- /dev/null +++ b/tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/number.js @@ -0,0 +1 @@ +module.exports = 5; diff --git a/tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/package.json b/tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/package.json new file mode 100644 index 0000000000..a9281c88ff --- /dev/null +++ b/tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/package.json @@ -0,0 +1,13 @@ +{ + "name": "@denotest/cjs-pkg-imports", + "version": "1.0.0", + "imports": { + "#crypto": { + "node": "./sub/dist/crypto.js", + "default": "./sub/dist/crypto.mjs" + }, + "#number": { + "node": "./number.js" + } + } +} \ No newline at end of file diff --git a/tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/sub/dist/crypto.js b/tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/sub/dist/crypto.js new file mode 100644 index 0000000000..70ffd5e5b8 --- /dev/null +++ b/tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/sub/dist/crypto.js @@ -0,0 +1 @@ +module.exports = require('node:crypto').webcrypto; diff --git a/tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/sub/dist/crypto.mjs b/tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/sub/dist/crypto.mjs new file mode 100644 index 0000000000..fe98f11545 --- /dev/null +++ b/tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/sub/dist/crypto.mjs @@ -0,0 +1 @@ +export default crypto;