fix(node): require of pkg json imports was broken (#22821)

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;

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

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<dyn std::error::Error>> {
       // A simple round trip benchmark for quick dev feedback.

ext/node/ops/require.rs

@@ -97,17 +97,16 @@ where
 {
   let fs = state.borrow::<FileSystemRc>();
   // 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::<P>(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<P>(
   state: &mut OpState,
-  #[string] parent_filename: String,
+  #[string] referrer_filename: String,
   #[string] request: String,
 ) -> Result<Option<String>, AnyError>
 where
   P: NodePermissions + 'static,
 {
-  let parent_path = PathBuf::from(&parent_filename);
-  ensure_read_permission::<P>(state, &parent_path)?;
+  let referrer_path = PathBuf::from(&referrer_filename);
+  ensure_read_permission::<P>(state, &referrer_path)?;
   let node_resolver = state.borrow::<Rc<NodeResolver>>();
   let permissions = state.borrow::<P>();
-  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<String, AnyError> {
+  let file_path = url_to_file_path(url)?;
+  Ok(file_path.to_string_lossy().to_string())
+}
+
+fn url_to_file_path(url: &Url) -> Result<PathBuf, AnyError> {
+  match url.to_file_path() {
+    Ok(file_path) => Ok(file_path),
+    Err(()) => {
+      deno_core::anyhow::bail!("failed to convert '{}' to file path", url)
+    }
+  }
+}

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<Option<PackageJson>, 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<Option<PackageJson>, 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<Option<PathBuf>, 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())?,
     );

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(

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 }

tests/testdata/npm/cjs_pkg_imports/main.ts

@@ -0,0 +1,3 @@
+import crypto from "npm:@denotest/cjs-pkg-imports";
+
+console.log(crypto);

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,
+};

tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/number.js

@@ -0,0 +1 @@
+module.exports = 5;

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"
+    }
+  }
+}

tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/sub/dist/crypto.js

@@ -0,0 +1 @@
+module.exports = require('node:crypto').webcrypto;

tests/testdata/npm/registry/@denotest/cjs-pkg-imports/1.0.0/sub/dist/crypto.mjs

@@ -0,0 +1 @@
+export default crypto;