Repository: oxc-project/oxc-resolver
Pull request #134 (2024-04-22 17:58:59)

fix: incorrect resolution when using shared resolvers with different `main_fields`
The modified test case

```rust
fn test() {
    let f = super::fixture().join("restrictions");

    let resolver1 = Resolver::new(ResolveOptions {
        main_fields: vec!["style".into()],
        ..ResolveOptions::default()
    });

    let resolution = resolver1.resolve(&f, "pck2").map(|r| r.full_path());
    assert_eq!(resolution, Ok(f.join("node_modules/pck2/index.css")));

    let resolver2 = resolver1.clone_with_options(ResolveOptions {
        main_fields: vec!["module".into(), "main".into()],
        ..ResolveOptions::default()
    });

    let resolution = resolver2.resolve(&f, "pck2").map(|r| r.full_path());
    assert_eq!(resolution, Ok(f.join("node_modules/pck2/module.js")));
}
```

failed to revolve due to package.json caching the value of `main_fields: vec!["style".into()]`, the second resolution `main_fields: vec!["module".into(), "main".into()]` would yield nothing because package.json#mainFields still holds the value of "style".

This PR changes caching the options to retrieving the values dynamically. I need to continue the work on all the other fields in https://github.com/oxc-project/oxc-resolver/issues/135

But I need to hotfix the `main_fields` change first for Rspack.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/oxc-project/oxc-resolver/pull/134?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=oxc-project) Report
Attention: Patch coverage is `91.66667%` with `1 lines` in your changes are missing coverage. Please review.
> Project coverage is 94.94%. Comparing base [(`ee22b66`)](https://app.codecov.io/gh/oxc-project/oxc-resolver/commit/ee22b6666997106aea6942629a3bd07fbfb24f97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=oxc-project) to head [(`0224a85`)](https://app.codecov.io/gh/oxc-project/oxc-resolver/pull/134?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=oxc-project).
> Report is 2 commits behind head on main.

| [Files](https://app.codecov.io/gh/oxc-project/oxc-resolver/pull/134?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=oxc-project) | Patch % | Lines |
|---|---|---|
| [src/lib.rs](https://app.codecov.io/gh/oxc-project/oxc-resolver/pull/134?src=pr&el=tree&filepath=src%2Flib.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=oxc-project#diff-c3JjL2xpYi5ycw==) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/oxc-project/oxc-resolver/pull/134?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=oxc-project) |

<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff           @@
##             main     #134   +/-   ##
=======================================
  Coverage   94.93%   94.94%           
=======================================
  Files          11       11           
  Lines        2212     2216    +4     
=======================================
+ Hits         2100     2104    +4     
  Misses        112      112           
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/oxc-project/oxc-resolver/pull/134?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=oxc-project).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=oxc-project).

codspeed-hq[bot]: ## [CodSpeed Performance Report](https://codspeed.io/oxc-project/oxc-resolver/branches/dont-cache-optins)

### Merging #134 will **not alter performance**

<sub>Comparing <code>dont-cache-optins</code> (0224a85) with <code>main</code> (db051d5)</sub>



### Summary

`✅ 2` untouched benchmarks






Patch:
diff --git a/src/lib.rs b/src/lib.rs
index 27e324e..4362dd8 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -515,7 +515,7 @@ impl<Fs: FileSystem> ResolverGeneric<Fs> {
                 cached_path.package_json(&self.cache.fs, &self.options, ctx)?
             {
                 // b. If "main" is a falsy value, GOTO 2.
-                for main_field in &package_json.main_fields {
+                for main_field in package_json.main_fields(&self.options.main_fields) {
                     // c. let M = X + (json main field)
                     let main_field_path = cached_path.path().normalize_with(main_field);
                     // d. LOAD_AS_FILE(M)
@@ -1149,7 +1149,7 @@ impl<Fs: FileSystem> ResolverGeneric<Fs> {
                         // 6. Otherwise, if packageSubpath is equal to ".", then
                         if subpath == "." {
                             // 1. If pjson.main is a string, then
-                            for main_field in &package_json.main_fields {
+                            for main_field in package_json.main_fields(&self.options.main_fields) {
                                 // 1. Return the URL resolution of main in packageURL.
                                 let path = cached_path.path().normalize_with(main_field);
                                 let cached_path = self.cache.value(&path);
diff --git a/src/package_json.rs b/src/package_json.rs
index 357e698..d4e075b 100644
--- a/src/package_json.rs
+++ b/src/package_json.rs
@@ -18,8 +18,7 @@ pub struct PackageJson {
     /// Realpath to `package.json`. Contains the `package.json` filename.
     pub realpath: PathBuf,
 
-    #[cfg(feature = "package_json_raw_json_api")]
-    pub(crate) raw_json: std::sync::Arc<serde_json::Value>,
+    raw_json: std::sync::Arc<serde_json::Value>,
 
     /// The "name" field defines your package's name.
     /// The "name" field can be used in addition to the "exports" field to self-reference a package using its name.
@@ -27,14 +26,6 @@ pub struct PackageJson {
     /// <https://nodejs.org/api/packages.html#name>
     pub name: Option<String>,
 
-    /// The "main" field defines the entry point of a package when imported by name via a node_modules lookup. Its value is a path.
-    /// When a package has an "exports" field, this will take precedence over the "main" field when importing the package by name.
-    ///
-    /// Values are dynamically added from [ResolveOptions::main_fields].
-    ///
-    /// <https://nodejs.org/api/packages.html#main>
-    pub main_fields: Vec<String>,
-
     /// The "exports" field allows defining the entry points of a package when imported by name loaded either via a node_modules lookup or a self-reference to its own name.
     ///
     /// <https://nodejs.org/api/packages.html#exports>
@@ -64,7 +55,6 @@ impl PackageJson {
         let mut raw_json: Value = serde_json::from_str(json)?;
         let mut package_json = Self::default();
 
-        package_json.main_fields.reserve_exact(options.main_fields.len());
         package_json.exports.reserve_exact(options.exports_fields.len());
         package_json.browser_fields.reserve_exact(options.alias_fields.len());
 
@@ -92,15 +82,6 @@ impl PackageJson {
                 .transpose()?
                 .map(Box::new);
 
-            // Dynamically create `main_fields`.
-            for main_field_key in &options.main_fields {
-                // Using `get` + `clone` instead of remove here
-                // because `main_fields` may contain `browser`, which is also used in `browser_fields.
-                if let Some(serde_json::Value::String(value)) = json_object.get(main_field_key) {
-                    package_json.main_fields.push(value.clone());
-                }
-            }
-
             // Dynamically create `browser_fields`.
             let dir = path.parent().unwrap();
             for object_path in &options.alias_fields {
@@ -138,10 +119,7 @@ impl PackageJson {
 
         package_json.path = path;
         package_json.realpath = realpath;
-        #[cfg(feature = "package_json_raw_json_api")]
-        {
-            package_json.raw_json = std::sync::Arc::new(raw_json);
-        }
+        package_json.raw_json = std::sync::Arc::new(raw_json);
         Ok(package_json)
     }
 
@@ -189,6 +167,23 @@ impl PackageJson {
         self.realpath.parent().unwrap()
     }
 
+    /// The "main" field defines the entry point of a package when imported by name via a node_modules lookup. Its value is a path.
+    ///
+    /// When a package has an "exports" field, this will take precedence over the "main" field when importing the package by name.
+    ///
+    /// Values are dynamically retrieved from [ResolveOptions::main_fields].
+    ///
+    /// <https://nodejs.org/api/packages.html#main>
+    pub(crate) fn main_fields<'a>(
+        &'a self,
+        main_fields: &'a [String],
+    ) -> impl Iterator<Item = &'a str> + '_ {
+        main_fields
+            .iter()
+            .filter_map(|main_field| self.raw_json.get(main_field))
+            .filter_map(|value| value.as_str())
+    }
+
     /// Resolve the request string for this package.json by looking at the `browser` field.
     ///
     /// # Errors


Test patch:
diff --git a/src/tests/main_field.rs b/src/tests/main_field.rs
index 98f7905..3cadc8c 100644
--- a/src/tests/main_field.rs
+++ b/src/tests/main_field.rs
@@ -6,19 +6,19 @@ use crate::{ResolveOptions, Resolver};
 fn test() {
     let f = super::fixture().join("restrictions");
 
-    let resolver = Resolver::new(ResolveOptions {
+    let resolver1 = Resolver::new(ResolveOptions {
         main_fields: vec!["style".into()],
         ..ResolveOptions::default()
     });
 
-    let resolution = resolver.resolve(&f, "pck2").map(|r| r.full_path());
+    let resolution = resolver1.resolve(&f, "pck2").map(|r| r.full_path());
     assert_eq!(resolution, Ok(f.join("node_modules/pck2/index.css")));
 
-    let resolver = Resolver::new(ResolveOptions {
+    let resolver2 = resolver1.clone_with_options(ResolveOptions {
         main_fields: vec!["module".into(), "main".into()],
         ..ResolveOptions::default()
     });
 
-    let resolution = resolver.resolve(&f, "pck2").map(|r| r.full_path());
+    let resolution = resolver2.resolve(&f, "pck2").map(|r| r.full_path());
     assert_eq!(resolution, Ok(f.join("node_modules/pck2/module.js")));
 }