fix(node): use closest package.json to resolve package.json imports (d…

…enoland#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,18 +163,21 @@ 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>,
 }
 
 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,
     )