fix(node): use closest package.json to resolve package.json imports (#21075)

cli/lsp/analysis.rs

@@ -8,6 +8,7 @@ use super::tsc;
 
 use crate::npm::CliNpmResolver;
 use crate::tools::lint::create_linter;
+use crate::util::path::specifier_to_file_path;
 
 use deno_ast::SourceRange;
 use deno_ast::SourceRangedForSpanned;
@@ -20,9 +21,10 @@ use deno_core::serde_json;
 use deno_core::serde_json::json;
 use deno_core::ModuleSpecifier;
 use deno_lint::rules::LintRule;
+use deno_runtime::deno_node::NodeResolver;
 use deno_runtime::deno_node::NpmResolver;
-use deno_runtime::deno_node::PackageJson;
 use deno_runtime::deno_node::PathClean;
+use deno_runtime::permissions::PermissionsContainer;
 use deno_semver::package::PackageReq;
 use import_map::ImportMap;
 use once_cell::sync::Lazy;
@@ -161,6 +163,7 @@ fn code_as_string(code: &Option<lsp::NumberOrString>) -> String {
 pub struct TsResponseImportMapper<'a> {
   documents: &'a Documents,
   maybe_import_map: Option<&'a ImportMap>,
+  node_resolver: Option<&'a NodeResolver>,
   npm_resolver: Option<&'a dyn CliNpmResolver>,
 }
 
@@ -168,11 +171,13 @@ impl<'a> TsResponseImportMapper<'a> {
   pub fn new(
     documents: &'a Documents,
     maybe_import_map: Option<&'a ImportMap>,
+    node_resolver: Option<&'a NodeResolver>,
     npm_resolver: Option<&'a dyn CliNpmResolver>,
   ) -> Self {
     Self {
       documents,
       maybe_import_map,
+      node_resolver,
       npm_resolver,
     }
   }
@@ -249,18 +254,14 @@ impl<'a> TsResponseImportMapper<'a> {
     &self,
     specifier: &ModuleSpecifier,
   ) -> Option<String> {
-    let specifier_path = specifier.to_file_path().ok()?;
-    let root_folder = self
-      .npm_resolver
-      .as_ref()
-      .and_then(|r| r.resolve_package_folder_from_path(specifier).ok())
+    let node_resolver = self.node_resolver?;
+    let package_json = node_resolver
+      .get_closest_package_json(specifier, &PermissionsContainer::allow_all())
+      .ok()
       .flatten()?;
-    let package_json_path = root_folder.join("package.json");
-    let package_json_text = std::fs::read_to_string(&package_json_path).ok()?;
-    let package_json =
-      PackageJson::load_from_string(package_json_path, package_json_text)
-        .ok()?;
+    let root_folder = package_json.path.parent()?;
 
+    let specifier_path = specifier_to_file_path(specifier).ok()?;
     let mut search_paths = vec![specifier_path.clone()];
     // TypeScript will provide a .js extension for quick fixes, so do
     // a search for the .d.ts file instead
@@ -271,7 +272,7 @@ impl<'a> TsResponseImportMapper<'a> {
     for search_path in search_paths {
       if let Some(exports) = &package_json.exports {
         if let Some(result) = try_reverse_map_package_json_exports(
-          &root_folder,
+          root_folder,
           &search_path,
           exports,
         ) {

cli/lsp/language_server.rs

@@ -2127,6 +2127,7 @@ impl Inner {
     TsResponseImportMapper::new(
       &self.documents,
       self.maybe_import_map.as_deref(),
+      self.npm.node_resolver.as_deref(),
       self.npm.resolver.as_deref(),
     )
   }

cli/npm/byonm.rs

@@ -91,16 +91,16 @@ impl NpmResolver for ByonmCliNpmResolver {
     fn inner(
       fs: &dyn FileSystem,
       name: &str,
-      package_root_path: &Path,
       referrer: &ModuleSpecifier,
       mode: NodeResolutionMode,
     ) -> Result<PathBuf, AnyError> {
+      let referrer_file = specifier_to_file_path(referrer)?;
       let types_pkg_name = if mode.is_types() && !name.starts_with("@types/") {
         Some(types_package_name(name))
       } else {
         None
       };
-      let mut current_folder = package_root_path;
+      let mut current_folder = referrer_file.parent().unwrap();
       loop {
         let node_modules_folder = if current_folder.ends_with("node_modules") {
           Cow::Borrowed(current_folder)
@@ -135,48 +135,10 @@ impl NpmResolver for ByonmCliNpmResolver {
       );
     }
 
-    let package_root_path =
-      self.resolve_package_folder_from_path(referrer)?.unwrap(); // todo(byonm): don't unwrap
-    let path = inner(&*self.fs, name, &package_root_path, referrer, mode)?;
+    let path = inner(&*self.fs, name, referrer, mode)?;
     Ok(self.fs.realpath_sync(&path)?)
   }
 
-  fn resolve_package_folder_from_path(
-    &self,
-    specifier: &deno_core::ModuleSpecifier,
-  ) -> Result<Option<PathBuf>, AnyError> {
-    let path = specifier.to_file_path().unwrap(); // todo(byonm): don't unwrap
-    let path = self.fs.realpath_sync(&path)?;
-    if self.in_npm_package(specifier) {
-      let mut path = path.as_path();
-      while let Some(parent) = path.parent() {
-        if parent
-          .file_name()
-          .and_then(|f| f.to_str())
-          .map(|s| s.to_ascii_lowercase())
-          .as_deref()
-          == Some("node_modules")
-        {
-          return Ok(Some(path.to_path_buf()));
-        } else {
-          path = parent;
-        }
-      }
-    } else {
-      // find the folder with a package.json
-      // todo(dsherret): not exactly correct, but good enough for a first pass
-      let mut path = path.as_path();
-      while let Some(parent) = path.parent() {
-        if self.fs.exists_sync(&parent.join("package.json")) {
-          return Ok(Some(parent.to_path_buf()));
-        } else {
-          path = parent;
-        }
-      }
-    }
-    Ok(None)
-  }
-
   fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool {
     specifier.scheme() == "file"
       && specifier

cli/npm/managed/mod.rs

@@ -505,24 +505,6 @@ impl NpmResolver for ManagedCliNpmResolver {
     Ok(path)
   }
 
-  fn resolve_package_folder_from_path(
-    &self,
-    specifier: &ModuleSpecifier,
-  ) -> Result<Option<PathBuf>, AnyError> {
-    let Some(path) = self
-      .fs_resolver
-      .resolve_package_folder_from_specifier(specifier)?
-    else {
-      return Ok(None);
-    };
-    log::debug!(
-      "Resolved package folder of {} to {}",
-      specifier,
-      path.display()
-    );
-    Ok(Some(path))
-  }
-
   fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool {
     let root_dir_url = self.fs_resolver.root_dir_url();
     debug_assert!(root_dir_url.as_str().ends_with('/'));

cli/tests/integration/npm_tests.rs

@@ -2505,3 +2505,28 @@ console.log(add(1, 2));
     .run();
   output.assert_matches_text("Check file:///[WILDCARD]/project-b/main.ts\n");
 }
+
+itest!(imports_package_json {
+  args: "run --node-modules-dir=false npm/imports_package_json/main.js",
+  output: "npm/imports_package_json/main.out",
+  envs: env_vars_for_npm_tests(),
+  http_server: true,
+});
+
+itest!(imports_package_json_import_not_defined {
+  args:
+    "run --node-modules-dir=false npm/imports_package_json/import_not_defined.js",
+  output: "npm/imports_package_json/import_not_defined.out",
+  envs: env_vars_for_npm_tests(),
+  exit_code: 1,
+  http_server: true,
+});
+
+itest!(imports_package_json_sub_path_import_not_defined {
+  args:
+    "run --node-modules-dir=false npm/imports_package_json/sub_path_import_not_defined.js",
+  output: "npm/imports_package_json/sub_path_import_not_defined.out",
+  envs: env_vars_for_npm_tests(),
+  exit_code: 1,
+  http_server: true,
+});

cli/tests/testdata/npm/imports_package_json/import_not_defined.js

@@ -0,0 +1,3 @@
+import data from "@denotest/imports-package-json/import-not-defined";
+
+console.log(data);

cli/tests/testdata/npm/imports_package_json/import_not_defined.out

@@ -0,0 +1,6 @@
+Download http://localhost:4545/npm/registry/@denotest/imports-package-json
+Download http://localhost:4545/npm/registry/@denotest/imports-package-json/1.0.0.tgz
+error: Could not resolve '#not-defined' from 'file:///[WILDCARD]/@denotest/imports-package-json/1.0.0/import_not_defined.js'.
+
+Caused by:
+    [ERR_PACKAGE_IMPORT_NOT_DEFINED] Package import specifier "#not-defined" is not defined in package [WILDCARD]package.json imported from [WILDCARD]import_not_defined.js

cli/tests/testdata/npm/imports_package_json/main.js

@@ -0,0 +1,4 @@
+import data from "@denotest/imports-package-json";
+
+console.log(data.hi);
+console.log(data.bye);

cli/tests/testdata/npm/imports_package_json/main.out

@@ -0,0 +1,4 @@
+Download http://localhost:4545/npm/registry/@denotest/imports-package-json
+Download http://localhost:4545/npm/registry/@denotest/imports-package-json/1.0.0.tgz
+hi
+bye

cli/tests/testdata/npm/imports_package_json/package.json

@@ -0,0 +1,6 @@
+{
+  "name": "my-test",
+  "dependencies": {
+    "@denotest/imports-package-json": "1.0.0"
+  }
+}

cli/tests/testdata/npm/imports_package_json/sub_path_import_not_defined.js

@@ -0,0 +1,3 @@
+import data from "@denotest/imports-package-json/sub-path-import-not-defined";
+
+console.log(data);

cli/tests/testdata/npm/imports_package_json/sub_path_import_not_defined.out

@@ -0,0 +1,6 @@
+Download http://localhost:4545/npm/registry/@denotest/imports-package-json
+Download http://localhost:4545/npm/registry/@denotest/imports-package-json/1.0.0.tgz
+error: Could not resolve '#hi' from 'file:///[WILDCARD]/@denotest/imports-package-json/1.0.0/sub_path/import_not_defined.js'.
+
+Caused by:
+    [ERR_PACKAGE_IMPORT_NOT_DEFINED] Package import specifier "#hi" is not defined in package [WILDCARD]sub_path[WILDCARD]package.json imported from [WILDCARD]import_not_defined.js

cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/hi.js

@@ -0,0 +1 @@
+export default "hi";

cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/import_not_defined.js

@@ -0,0 +1,3 @@
+import notDefined from "#not-defined";
+
+export default notDefined;

cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/main.js

@@ -0,0 +1,7 @@
+import hi from "#hi";
+import bye from "./sub_path/main.js";
+
+export default {
+  hi, 
+  bye,
+};

cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/package.json

@@ -0,0 +1,16 @@
+{
+  "name": "imports-package-json",
+  "type": "module",
+  "version": "1.0.0",
+  "description": "",
+  "license": "ISC",
+  "author": "",
+  "exports": {
+    ".": "./main.js",
+    "./import-not-defined": "./import_not_defined.js",
+    "./sub-path-import-not-defined": "./sub_path/import_not_defined.js"
+  },
+  "imports": {
+    "#hi": "./hi.js"
+  }
+}

cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/sub_path/bye.js

@@ -0,0 +1 @@
+export default "bye";

cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/sub_path/import_not_defined.js

@@ -0,0 +1,4 @@
+// this won't be defined in the closest package.json and will fail
+import hi from "#hi";
+
+export default hi;

cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/sub_path/main.js

@@ -0,0 +1,3 @@
+import bye from "#bye";
+
+export default bye;

cli/tests/testdata/npm/registry/@denotest/imports-package-json/1.0.0/sub_path/package.json

@@ -0,0 +1,6 @@
+{
+  "type": "module",
+  "imports": {
+    "#bye": "./bye.js"
+  }
+}

ext/node/errors.rs

@@ -138,11 +138,12 @@ pub fn err_package_import_not_defined(
   base: &str,
 ) -> AnyError {
   let mut msg = format!(
-    "[ERR_PACKAGE_IMPORT_NOT_DEFINED] Package import specifier \"{specifier}\" is not defined in"
+    "[ERR_PACKAGE_IMPORT_NOT_DEFINED] Package import specifier \"{specifier}\" is not defined"
   );
 
   if let Some(package_path) = package_path {
-    msg = format!("{msg} in package {package_path}package.json");
+    let pkg_json_path = PathBuf::from(package_path).join("package.json");
+    msg = format!("{} in package {}", msg, pkg_json_path.display());
   }
 
   msg = format!("{msg} imported from {base}");

ext/node/lib.rs

@@ -81,12 +81,6 @@ pub trait NpmResolver: std::fmt::Debug + MaybeSend + MaybeSync {
     mode: NodeResolutionMode,
   ) -> Result<PathBuf, AnyError>;
 
-  /// Resolves the npm package folder path from the specified path.
-  fn resolve_package_folder_from_path(
-    &self,
-    specifier: &ModuleSpecifier,
-  ) -> Result<Option<PathBuf>, AnyError>;
-
   fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool;
 
   fn in_npm_package_at_path(&self, path: &Path) -> bool {

ext/node/ops/require.rs

@@ -389,7 +389,7 @@ where
   let node_resolver = state.borrow::<Rc<NodeResolver>>();
   let permissions = state.borrow::<P>();
   let pkg = node_resolver
-    .get_package_scope_config(
+    .get_closest_package_json(
       &Url::from_file_path(parent_path.unwrap()).unwrap(),
       permissions,
     )

ext/node/polyfills/internal/errors.ts

@@ -2421,7 +2421,7 @@ export class ERR_INVALID_PACKAGE_TARGET extends NodeError {
     if (key === ".") {
       assert(isImport === false);
       msg = `Invalid "exports" main target ${JSON.stringify(target)} defined ` +
-        `in the package config ${pkgPath}package.json${
+        `in the package config ${displayJoin(pkgPath, "package.json")}${
           base ? ` imported from ${base}` : ""
         }${relError ? '; targets must start with "./"' : ""}`;
     } else {
@@ -2429,9 +2429,11 @@ export class ERR_INVALID_PACKAGE_TARGET extends NodeError {
         JSON.stringify(
           target,
         )
-      } defined for '${key}' in the package config ${pkgPath}package.json${
-        base ? ` imported from ${base}` : ""
-      }${relError ? '; targets must start with "./"' : ""}`;
+      } defined for '${key}' in the package config ${
+        displayJoin(pkgPath, "package.json")
+      }${base ? ` imported from ${base}` : ""}${
+        relError ? '; targets must start with "./"' : ""
+      }`;
     }
     super("ERR_INVALID_PACKAGE_TARGET", msg);
   }
@@ -2444,7 +2446,9 @@ export class ERR_PACKAGE_IMPORT_NOT_DEFINED extends NodeTypeError {
     base: string,
   ) {
     const msg = `Package import specifier "${specifier}" is not defined${
-      packagePath ? ` in package ${packagePath}package.json` : ""
+      packagePath
+        ? ` in package ${displayJoin(packagePath, "package.json")}`
+        : ""
     } imported from ${base}`;
 
     super("ERR_PACKAGE_IMPORT_NOT_DEFINED", msg);
@@ -2455,14 +2459,13 @@ export class ERR_PACKAGE_PATH_NOT_EXPORTED extends NodeError {
   constructor(subpath: string, pkgPath: string, basePath?: string) {
     let msg: string;
     if (subpath === ".") {
-      msg = `No "exports" main defined in ${pkgPath}package.json${
-        basePath ? ` imported from ${basePath}` : ""
-      }`;
+      msg = `No "exports" main defined in ${
+        displayJoin(pkgPath, "package.json")
+      }${basePath ? ` imported from ${basePath}` : ""}`;
     } else {
-      msg =
-        `Package subpath '${subpath}' is not defined by "exports" in ${pkgPath}package.json${
-          basePath ? ` imported from ${basePath}` : ""
-        }`;
+      msg = `Package subpath '${subpath}' is not defined by "exports" in ${
+        displayJoin(pkgPath, "package.json")
+      }${basePath ? ` imported from ${basePath}` : ""}`;
     }
 
     super("ERR_PACKAGE_PATH_NOT_EXPORTED", msg);
@@ -2621,6 +2624,12 @@ function determineSpecificType(value: any) {
   return `type ${typeof value} (${inspected})`;
 }
 
+// Non-robust path join
+function displayJoin(dir: string, fileName: string) {
+  const sep = dir.includes("\\") ? "\\" : "/";
+  return dir.endsWith(sep) ? dir + fileName : dir + sep + fileName;
+}
+
 export { codes, genericNodeError, hideStackFrames };
 
 export default {

ext/node/resolution.rs

@@ -543,7 +543,7 @@ impl NodeResolver {
 
     let mut package_json_path = None;
     if let Some(package_config) =
-      self.get_package_scope_config(referrer, permissions)?
+      self.get_closest_package_json(referrer, permissions)?
     {
       if package_config.exists {
         package_json_path = Some(package_config.path.clone());
@@ -977,12 +977,12 @@ impl NodeResolver {
     let (package_name, package_subpath, _is_scoped) =
       parse_npm_pkg_name(specifier, referrer)?;
 
-    // ResolveSelf
     let Some(package_config) =
-      self.get_package_scope_config(referrer, permissions)?
+      self.get_closest_package_json(referrer, permissions)?
     else {
       return Ok(None);
     };
+    // ResolveSelf
     if package_config.exists
       && package_config.name.as_ref() == Some(&package_name)
     {
@@ -1086,24 +1086,7 @@ impl NodeResolver {
     }
   }
 
-  pub(super) fn get_package_scope_config(
-    &self,
-    referrer: &ModuleSpecifier,
-    permissions: &dyn NodePermissions,
-  ) -> Result<Option<PackageJson>, AnyError> {
-    let Some(root_folder) = self
-      .npm_resolver
-      .resolve_package_folder_from_path(referrer)?
-    else {
-      return Ok(None);
-    };
-    let package_json_path = root_folder.join("package.json");
-    self
-      .load_package_json(permissions, package_json_path)
-      .map(Some)
-  }
-
-  pub(super) fn get_closest_package_json(
+  pub fn get_closest_package_json(
     &self,
     url: &ModuleSpecifier,
     permissions: &dyn NodePermissions,
@@ -1121,7 +1104,9 @@ impl NodeResolver {
     &self,
     url: &ModuleSpecifier,
   ) -> Result<Option<PathBuf>, AnyError> {
-    let file_path = url.to_file_path().unwrap();
+    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())?,
     );
@@ -1130,15 +1115,8 @@ impl NodeResolver {
     if self.fs.exists_sync(&package_json_path) {
       return Ok(Some(package_json_path));
     }
-    let Some(root_pkg_folder) =
-      self.npm_resolver.resolve_package_folder_from_path(
-        &ModuleSpecifier::from_directory_path(current_dir).unwrap(),
-      )?
-    else {
-      return Ok(None);
-    };
-    while current_dir.starts_with(&root_pkg_folder) {
-      current_dir = current_dir.parent().unwrap();
+    while let Some(parent) = current_dir.parent() {
+      current_dir = parent;
       let package_json_path = current_dir.join("package.json");
       if self.fs.exists_sync(&package_json_path) {
         return Ok(Some(package_json_path));