Repository: oxc-project/oxc-resolver (rust)

Issue:
bug: not returning the correct package.json where the path is resolved from
Currently, `Resolution::package_json` returns the nearest package.json from that resolved path.
To reproduce, you can edit `examples/resolver.rs` to print `package.json()` and run `cargo run --example resolver -- "/path/to/a/place/that/has/vue/installed" "vue/server-renderer"` (`vue/server-renderer/package.json` is output instead of `vue/package.json`).

But in some cases, you want the nearest package.json with `name` field. For example, reading the `exports` / `imports` field, or reading `dependencies` field. IIRC `sideEffects` field is also the package.json with `name` field.


Pull request:
fix: return the enclosing `package.json` if it is inside `node_modules`
closes #396

A package may contain package.json inside its subdirectories, they must not participate in module resolution. 

Example:

In the published vue package, it contains

```
vue/package.json
vue/server-renderer/package.json
vue/server-renderer/index.mjs
```

When requesting `vue/server-renderer`, it now esolves to `vue/server-renderer/index.mjs` with `vue/package.json`. It returned `vue/server-renderer/package.json` before this PR.

`vue/server-renderer/package.json` does not participate in module resolution according to the node.js module resolution algorithm so the returned package.json resolution should not be `vue/server-renderer/package.json`.

---

Note: There is an undefined behavior for resolving the `sideEffects` field.

```
vue/package.json (sideEffects: true)
vue/server-renderer/package.json (sideEffects: true)
vue/server-renderer/index.mjs
```

What should `sideEffects` return when requesting `vue/server-renderer`?

* All current bundlers observe `vue/server-renderer/package.json (sideEffects: true)` -resolvers assume a package has only a root package.json so they just find the closes package.json starting from the end of the resolved path
* But `vue/server-renderer/package.json` never participated in module resolution, why would we end up reading its `sideEffects` field?

Patch:
diff --git a/fixtures/misc/node_modules/package-json-nested/foo/bar/index.js b/fixtures/misc/node_modules/package-json-nested/foo/bar/index.js
new file mode 100644
index 0000000..e69de29
diff --git a/fixtures/misc/node_modules/package-json-nested/foo/bar/package.json b/fixtures/misc/node_modules/package-json-nested/foo/bar/package.json
new file mode 100644
index 0000000..694594c
--- /dev/null
+++ b/fixtures/misc/node_modules/package-json-nested/foo/bar/package.json
@@ -0,0 +1,3 @@
+{
+  "name": "bar"
+}
diff --git a/fixtures/misc/node_modules/package-json-nested/foo/index.js b/fixtures/misc/node_modules/package-json-nested/foo/index.js
new file mode 100644
index 0000000..e69de29
diff --git a/fixtures/misc/node_modules/package-json-nested/foo/package.json b/fixtures/misc/node_modules/package-json-nested/foo/package.json
new file mode 100644
index 0000000..bde99de
--- /dev/null
+++ b/fixtures/misc/node_modules/package-json-nested/foo/package.json
@@ -0,0 +1,3 @@
+{
+  "name": "foo"
+}
diff --git a/fixtures/misc/node_modules/package-json-nested/index.js b/fixtures/misc/node_modules/package-json-nested/index.js
new file mode 100644
index 0000000..e69de29
diff --git a/fixtures/misc/node_modules/package-json-nested/package.json b/fixtures/misc/node_modules/package-json-nested/package.json
new file mode 100644
index 0000000..755e83a
--- /dev/null
+++ b/fixtures/misc/node_modules/package-json-nested/package.json
@@ -0,0 +1,3 @@
+{
+  "name": "package-json-nested"
+}
diff --git a/src/cache.rs b/src/cache.rs
index 2156661..b47f1fc 100644
--- a/src/cache.rs
+++ b/src/cache.rs
@@ -62,6 +62,8 @@ pub trait CachedPath: Sized {
 
     fn parent(&self) -> Option<&Self>;
 
+    fn node_modules(&self) -> Option<&Self>;
+
     fn module_directory<C: Cache<Cp = Self>>(
         &self,
         module_name: &str,
diff --git a/src/fs_cache.rs b/src/fs_cache.rs
index 731a675..b760d15 100644
--- a/src/fs_cache.rs
+++ b/src/fs_cache.rs
@@ -314,6 +314,10 @@ impl CachedPath for FsCachedPath {
         cache.is_dir(&cached_path, ctx).then_some(cached_path)
     }
 
+    fn node_modules(&self) -> Option<&Self> {
+        self.node_modules.get().and_then(|o| o.as_ref())
+    }
+
     fn cached_node_modules<C: Cache<Cp = Self>>(&self, cache: &C, ctx: &mut Ctx) -> Option<Self> {
         self.node_modules.get_or_init(|| self.module_directory("node_modules", cache, ctx)).clone()
     }
diff --git a/src/lib.rs b/src/lib.rs
index 48163cc..01d4ca2 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -75,7 +75,7 @@ use std::{
     borrow::Cow,
     cmp::Ordering,
     ffi::OsStr,
-    fmt,
+    fmt, iter,
     path::{Component, Path, PathBuf},
     sync::Arc,
 };
@@ -266,9 +266,8 @@ impl<C: Cache> ResolverGeneric<C> {
         let path = self.load_realpath(&cached_path)?;
         // enhanced-resolve: restrictions
         self.check_restrictions(&path)?;
-        let package_json =
-            cached_path.find_package_json(&self.options, self.cache.as_ref(), ctx)?;
-        if let Some((_, package_json)) = &package_json {
+        let package_json = self.find_package_json_for_a_package(&cached_path, ctx)?;
+        if let Some(package_json) = &package_json {
             // path must be inside the package.
             debug_assert!(path.starts_with(package_json.directory()));
         }
@@ -276,10 +275,42 @@ impl<C: Cache> ResolverGeneric<C> {
             path,
             query: ctx.query.take(),
             fragment: ctx.fragment.take(),
-            package_json: package_json.map(|(_, p)| p),
+            package_json,
         })
     }
 
+    fn find_package_json_for_a_package(
+        &self,
+        cached_path: &C::Cp,
+        ctx: &mut Ctx,
+    ) -> Result<Option<Arc<C::Pj>>, ResolveError> {
+        // Algorithm:
+        // Find `node_modules/package/package.json`
+        // or the first package.json if the path is not inside node_modules.
+        let inside_node_modules = iter::successors(Some(cached_path), |cp| cp.parent())
+            .any(|cp| cp.node_modules().is_some());
+        if inside_node_modules {
+            let mut last = None;
+            for cp in iter::successors(Some(cached_path), |cp| cp.parent()) {
+                if cp.node_modules().is_some() {
+                    break;
+                }
+                if self.cache.is_dir(cp, ctx) {
+                    if let Some((_, package_json)) =
+                        self.cache.get_package_json(cp, &self.options, ctx)?
+                    {
+                        last = Some(package_json);
+                    }
+                }
+            }
+            Ok(last)
+        } else {
+            cached_path
+                .find_package_json(&self.options, self.cache.as_ref(), ctx)
+                .map(|result| result.map(|(_, p)| p))
+        }
+    }
+
     /// require(X) from module at path Y
     ///
     /// X: specifier


Test patch:
diff --git a/fixtures/enhanced_resolve/test/fixtures/multiple_modules/node_modules/m1/package.json b/fixtures/enhanced_resolve/test/fixtures/multiple_modules/node_modules/m1/package.json
new file mode 100644
index 0000000..0967ef4
--- /dev/null
+++ b/fixtures/enhanced_resolve/test/fixtures/multiple_modules/node_modules/m1/package.json
@@ -0,0 +1,1 @@
+{}
diff --git a/fixtures/enhanced_resolve/test/fixtures/node_modules/complexm/package.json b/fixtures/enhanced_resolve/test/fixtures/node_modules/complexm/package.json
new file mode 100644
index 0000000..0967ef4
--- /dev/null
+++ b/fixtures/enhanced_resolve/test/fixtures/node_modules/complexm/package.json
@@ -0,0 +1,1 @@
+{}
diff --git a/fixtures/enhanced_resolve/test/fixtures/node_modules/m1/package.json b/fixtures/enhanced_resolve/test/fixtures/node_modules/m1/package.json
new file mode 100644
index 0000000..0967ef4
--- /dev/null
+++ b/fixtures/enhanced_resolve/test/fixtures/node_modules/m1/package.json
@@ -0,0 +1,1 @@
+{}
diff --git a/fixtures/enhanced_resolve/test/fixtures/node_modules/m2/package.json b/fixtures/enhanced_resolve/test/fixtures/node_modules/m2/package.json
new file mode 100644
index 0000000..0967ef4
--- /dev/null
+++ b/fixtures/enhanced_resolve/test/fixtures/node_modules/m2/package.json
@@ -0,0 +1,1 @@
+{}
diff --git a/src/tests/dependencies.rs b/src/tests/dependencies.rs
index 3442b9d..5584a1b 100644
--- a/src/tests/dependencies.rs
+++ b/src/tests/dependencies.rs
@@ -52,6 +52,7 @@ mod windows {
                     // "/a/b/c/package.json",
                     "/a/b/package.json",
                     "/a/package.json",
+                    "/a/node_modules/package.json",
                     "/package.json",
                     // missing modules directories
                     "/a/b/c",
@@ -99,8 +100,11 @@ mod windows {
             assert_eq!(resolved_path, Ok(PathBuf::from(result)));
             let file_dependencies = file_dependencies.iter().map(PathBuf::from).collect();
             let missing_dependencies = missing_dependencies.iter().map(PathBuf::from).collect();
-            assert_eq!(ctx.file_dependencies, file_dependencies, "{name}");
-            assert_eq!(ctx.missing_dependencies, missing_dependencies, "{name}");
+            assert_eq!(ctx.file_dependencies, file_dependencies, "{name} file_dependencies");
+            assert_eq!(
+                ctx.missing_dependencies, missing_dependencies,
+                "{name} missing_dependencies"
+            );
         }
     }
 }
diff --git a/src/tests/mod.rs b/src/tests/mod.rs
index c156795..66debfb 100644
--- a/src/tests/mod.rs
+++ b/src/tests/mod.rs
@@ -12,6 +12,7 @@ mod incorrect_description_file;
 mod main_field;
 mod memory_fs;
 mod missing;
+mod package_json;
 #[cfg(feature = "yarn_pnp")]
 mod pnp;
 mod resolution;
diff --git a/src/tests/package_json.rs b/src/tests/package_json.rs
new file mode 100644
index 0000000..8575763
--- /dev/null
+++ b/src/tests/package_json.rs
@@ -0,0 +1,28 @@
+//! Tests for `Resolution::package_json`.
+
+use crate::Resolver;
+
+#[test]
+fn test() {
+    let f = super::fixture_root().join("misc");
+
+    let resolver = Resolver::default();
+
+    let data = [
+        (f.clone(), "package-json-nested"),
+        // Nested package.json do not participate in module resolution.
+        (f.clone(), "package-json-nested/foo"),
+        (f.clone(), "package-json-nested/foo/bar"),
+    ];
+
+    let resolved_package_json_path = f.join("node_modules/package-json-nested/package.json");
+
+    for (path, request) in data {
+        let package_json =
+            resolver.resolve(&path, request).ok().and_then(|f| f.package_json().cloned());
+        let package_json_path = package_json.as_ref().map(|p| &p.path);
+        let package_json_name = package_json.as_ref().and_then(|p| p.name.as_deref());
+        assert_eq!(package_json_path, Some(&resolved_package_json_path), "{path:?} {request}");
+        assert_eq!(package_json_name, Some("package-json-nested"), "{path:?} {request}");
+    }
+}
diff --git a/src/tests/resolve.rs b/src/tests/resolve.rs
index 17ea785..3e3ebc6 100644
--- a/src/tests/resolve.rs
+++ b/src/tests/resolve.rs
@@ -1,6 +1,6 @@
 //! <https://github.com/webpack/enhanced-resolve/blob/main/test/resolve.test.js>
 
-use crate::{ResolveError, ResolveOptions, Resolver};
+use crate::{Resolution, ResolveError, ResolveOptions, Resolver};
 
 #[test]
 fn resolve() {
@@ -53,8 +53,14 @@ fn resolve() {
     ];
 
     for (comment, path, request, expected) in pass {
-        let resolved_path = resolver.resolve(&path, request).map(|r| r.full_path());
-        assert_eq!(resolved_path, Ok(expected), "{comment} {path:?} {request}");
+        let resolution = resolver.resolve(&path, request).ok();
+        let resolved_path = resolution.as_ref().map(Resolution::full_path);
+        let resolved_package_json =
+            resolution.as_ref().and_then(|r| r.package_json()).map(|p| p.path.clone());
+        if expected.to_str().unwrap().contains("node_modules") {
+            assert!(resolved_package_json.is_some(), "{comment} {path:?} {request}");
+        }
+        assert_eq!(resolved_path, Some(expected), "{comment} {path:?} {request}");
     }
 }
 
diff --git a/src/tests/scoped_packages.rs b/src/tests/scoped_packages.rs
index 8258f00..0854873 100644
--- a/src/tests/scoped_packages.rs
+++ b/src/tests/scoped_packages.rs
@@ -1,6 +1,6 @@
 //! <https://github.com/webpack/enhanced-resolve/blob/main/test/scoped-packages.test.js>
 
-use crate::{ResolveOptions, Resolver};
+use crate::{Resolution, ResolveOptions, Resolver};
 
 #[test]
 fn scoped_packages() {
@@ -13,13 +13,18 @@ fn scoped_packages() {
 
     #[rustfmt::skip]
     let pass = [
-        ("main field should work", f.clone(), "@scope/pack1", f.join("./node_modules/@scope/pack1/main.js")),
-        ("browser field should work", f.clone(), "@scope/pack2", f.join("./node_modules/@scope/pack2/main.js")),
-        ("folder request should work", f.clone(), "@scope/pack2/lib", f.join("./node_modules/@scope/pack2/lib/index.js"))
+        ("main field should work", f.clone(), "@scope/pack1", "@scope/pack1", f.join("./node_modules/@scope/pack1/main.js")),
+        ("browser field should work", f.clone(), "@scope/pack2", "@scope/pack2", f.join("./node_modules/@scope/pack2/main.js")),
+        ("folder request should work", f.clone(), "@scope/pack2/lib", "@scope/pack2", f.join("./node_modules/@scope/pack2/lib/index.js"))
     ];
 
-    for (comment, path, request, expected) in pass {
-        let resolved_path = resolver.resolve(&f, request).map(|r| r.full_path());
-        assert_eq!(resolved_path, Ok(expected), "{comment} {path:?} {request}");
+    for (comment, path, request, package, expected) in pass {
+        let resolution = resolver.resolve(&path, request).ok();
+        let resolved_path = resolution.as_ref().map(Resolution::full_path);
+        let resolved_package_json =
+            resolution.as_ref().and_then(|r| r.package_json()).map(|p| p.path.clone());
+        assert_eq!(resolved_path, Some(expected), "{comment} {path:?} {request}");
+        let package_json_path = f.join("node_modules").join(package).join("package.json");
+        assert_eq!(resolved_package_json, Some(package_json_path), "{path:?} {request}");
     }
 }
diff --git a/src/tests/simple.rs b/src/tests/simple.rs
index 4fc8831..1d823f2 100644
--- a/src/tests/simple.rs
+++ b/src/tests/simple.rs
@@ -2,7 +2,7 @@
 
 use std::env;
 
-use crate::Resolver;
+use crate::{Resolution, Resolver};
 
 #[test]
 fn simple() {
@@ -41,8 +41,13 @@ fn dashed_name() {
     ];
 
     for (path, request, expected) in data {
-        let resolved_path = resolver.resolve(&path, request).map(|f| f.full_path());
-        assert_eq!(resolved_path, Ok(expected), "{path:?} {request}");
+        let resolution = resolver.resolve(&path, request).ok();
+        let resolved_path = resolution.as_ref().map(Resolution::full_path);
+        let resolved_package_json =
+            resolution.as_ref().and_then(|r| r.package_json()).map(|p| p.path.clone());
+        assert_eq!(resolved_path, Some(expected), "{path:?} {request}");
+        let package_json_path = f.join("node_modules").join(request).join("package.json");
+        assert_eq!(resolved_package_json, Some(package_json_path), "{path:?} {request}");
     }
 }