Repository: oxc-project/oxc-resolver
Pull request #103 (2024-03-07 08:33:04)

refactor: selectively parse package_json fields instead of parsing everything
so we don't end up parsing unwanted fields and produce errors we don't
care about.

Boshen: * **#103** <a href="https://app.graphite.dev/github/pr/oxc-project/oxc-resolver/103?utm_source=stack-comment-icon" target="_blank"><img src="https://static.graphite.dev/graphite-32x32-black.png" alt="Graphite" width="10px" height="10px"/></a> 👈
* `main`

This stack of pull requests is managed by Graphite. <a href="https://stacking.dev/?utm_source=stack-comment">Learn more about stacking.</a>
<h2></h2>

 Join @Boshen and the rest of your teammates on <a href="https://graphite.dev?utm-source=stack-comment"><img src="https://static.graphite.dev/graphite-32x32-black.png" alt="Graphite" width="11px" height="11px"/> <b>Graphite</b></a>
  <!-- Current dependencies on/for this PR: -->
codspeed-hq[bot]: ## [CodSpeed Performance Report](https://codspeed.io/oxc-project/oxc-resolver/branches/03-07-refactor_selectively_parse_package_json_fields_instead_of_parsing_everything)

### Merging #103 will **not alter performance**

<sub>Comparing <code>03-07-refactor_selectively_parse_package_json_fields_instead_of_parsing_everything</code> (6b99584) with <code>main</code> (fa51208)</sub>



### Summary

`✅ 2` untouched benchmarks






Patch:
diff --git a/napi/src/lib.rs b/napi/src/lib.rs
index fd0d33a..cad1b72 100644
--- a/napi/src/lib.rs
+++ b/napi/src/lib.rs
@@ -157,7 +157,6 @@ impl ResolverFactory {
                 .unwrap_or(default.roots),
             symlinks: op.symlinks.unwrap_or(default.symlinks),
             builtin_modules: op.builtin_modules.unwrap_or(default.builtin_modules),
-            remove_unused_fields: op.remove_unused_fields.unwrap_or(default.remove_unused_fields),
         }
     }
 }
diff --git a/napi/src/options.rs b/napi/src/options.rs
index 9fe4fce..f1ecb14 100644
--- a/napi/src/options.rs
+++ b/napi/src/options.rs
@@ -141,10 +141,6 @@ pub struct NapiResolveOptions {
     ///
     /// Default `false`
     pub builtin_modules: Option<bool>,
-
-    /// When enabled, will attempt to reduce memory usage by clearing data and fields
-    /// that are not relevant to the current process, and should not be held in memory.
-    pub remove_unused_fields: Option<bool>,
 }
 
 #[napi]
diff --git a/src/lib.rs b/src/lib.rs
index dca1738..89af0de 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -440,7 +440,9 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {
             return Ok(None);
         };
         // 3. If the SCOPE/package.json "imports" is null or undefined, return.
-        if package_json.imports.is_empty() {
+        if package_json.imports.is_none()
+            || package_json.imports.as_ref().is_some_and(|imports| imports.is_empty())
+        {
             return Ok(None);
         }
         // 4. let MATCH = PACKAGE_IMPORTS_RESOLVE(X, pathToFileURL(SCOPE), ["node", "require"]) defined in the ESM resolver.
@@ -753,7 +755,6 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {
         if !package_json.exports.is_empty() {
             // 4. If the SCOPE/package.json "name" is not the first segment of X, return.
             if let Some(subpath) = package_json
-                .data
                 .name
                 .as_ref()
                 .and_then(|package_name| Self::strip_package_name(specifier, package_name))
@@ -1266,16 +1267,18 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {
         // 2. If pjson.imports is a non-null Object, then
 
         // 1. Let resolved be the result of PACKAGE_IMPORTS_EXPORTS_RESOLVE( specifier, pjson.imports, packageURL, true, conditions).
-        if let Some(path) = self.package_imports_exports_resolve(
-            specifier,
-            &package_json.imports,
-            package_json.directory(),
-            /* is_imports */ true,
-            &self.options.condition_names,
-            ctx,
-        )? {
-            // 2. If resolved is not null or undefined, return resolved.
-            return Ok(path);
+        if let Some(imports) = &package_json.imports {
+            if let Some(path) = self.package_imports_exports_resolve(
+                specifier,
+                imports,
+                package_json.directory(),
+                /* is_imports */ true,
+                &self.options.condition_names,
+                ctx,
+            )? {
+                // 2. If resolved is not null or undefined, return resolved.
+                return Ok(path);
+            }
         }
 
         // 5. Throw a Package Import Not Defined error.
diff --git a/src/options.rs b/src/options.rs
index 704c88c..2c4ef7b 100644
--- a/src/options.rs
+++ b/src/options.rs
@@ -133,10 +133,6 @@ pub struct ResolveOptions {
     ///
     /// Default `false`
     pub builtin_modules: bool,
-
-    /// When enabled, will attempt to reduce memory usage by clearing data and fields
-    /// that are not relevant to the current process, and should not be held in memory.
-    pub remove_unused_fields: bool,
 }
 
 impl ResolveOptions {
@@ -377,7 +373,6 @@ impl Default for ResolveOptions {
             roots: vec![],
             symlinks: true,
             builtin_modules: false,
-            remove_unused_fields: true,
         }
     }
 }
diff --git a/src/package_json.rs b/src/package_json.rs
index feb04fb..436515d 100644
--- a/src/package_json.rs
+++ b/src/package_json.rs
@@ -6,31 +6,28 @@ use std::{
     sync::Arc,
 };
 
-use nodejs_package_json::{
-    BrowserField, ImportExportField, ImportExportMap, PackageJson as BasePackageJson,
-};
-use rustc_hash::FxHashMap;
+use nodejs_package_json::{BrowserField, ImportExportField, ImportExportMap};
 use serde::Deserialize;
 use serde_json::Value;
 
 use crate::{path::PathUtil, ResolveError, ResolveOptions};
 
 /// Deserialized package.json
-#[derive(Debug, Default, Deserialize)]
+#[derive(Debug, Default)]
 pub struct PackageJson {
     /// Path to `package.json`. Contains the `package.json` filename.
-    #[serde(skip)]
     pub path: PathBuf,
 
     /// Realpath to `package.json`. Contains the `package.json` filename.
-    #[serde(skip)]
     pub realpath: PathBuf,
 
-    #[serde(skip)]
     pub(crate) raw_json: Arc<serde_json::Value>,
 
-    /// The deserialized `package.json`.
-    pub data: BasePackageJson,
+    /// 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.
+    ///
+    /// <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.
@@ -38,26 +35,22 @@ pub struct PackageJson {
     /// Values are dynamically added from [ResolveOptions::main_fields].
     ///
     /// <https://nodejs.org/api/packages.html#main>
-    #[serde(skip)]
     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>
-    #[serde(skip)]
     pub exports: Vec<ImportExportField>,
 
     /// In addition to the "exports" field, there is a package "imports" field to create private mappings that only apply to import specifiers from within the package itself.
     ///
     /// <https://nodejs.org/api/packages.html#subpath-imports>
-    #[serde(default)]
-    pub imports: Box<ImportExportMap>,
+    pub imports: Option<Box<ImportExportMap>>,
 
     /// The "browser" field is provided by a module author as a hint to javascript bundlers or component tools when packaging modules for client side use.
     /// Multiple values are configured by [ResolveOptions::alias_fields].
     ///
     /// <https://github.com/defunctzombie/package-browser-field-spec>
-    #[serde(skip)]
     pub browser_fields: Vec<BrowserField>,
 }
 
@@ -71,127 +64,85 @@ impl PackageJson {
         options: &ResolveOptions,
     ) -> Result<Self, serde_json::Error> {
         let mut raw_json: Value = serde_json::from_str(json)?;
-        let mut data: BasePackageJson = serde_json::from_value(raw_json.clone())?;
-
         let mut package_json = Self::default();
+
         package_json.main_fields.reserve_exact(options.main_fields.len());
-        package_json.browser_fields.reserve_exact(options.alias_fields.len());
         package_json.exports.reserve_exact(options.exports_fields.len());
+        package_json.browser_fields.reserve_exact(options.alias_fields.len());
 
-        // Dynamically create `main_fields`.
-        for main_field_key in &options.main_fields {
-            match main_field_key.as_str() {
-                "main" => {
-                    if let Some(main_field) = &data.main {
-                        package_json.main_fields.push(main_field.to_string_lossy().to_string());
-                    }
-                }
-                "module" => {
-                    if let Some(module_field) = &data.module {
-                        package_json.main_fields.push(module_field.to_string_lossy().to_string());
-                    }
-                }
-                "browser" => {
-                    if let Some(BrowserField::String(browser_field)) = &data.browser {
-                        package_json.main_fields.push(browser_field.to_owned());
-                    }
-                }
-                field => {
-                    // Using `get` + `clone` instead of remove here
-                    // because `main_fields` may contain `browser`, which is also used in `browser_fields.
-                    if let Some(Value::String(value)) = data.other_fields.get(field) {
-                        package_json.main_fields.push(value.clone());
-                    }
-                }
-            };
-        }
-
-        // Dynamically create `browser_fields`.
-        let dir = path.parent().unwrap();
-        for object_path in &options.alias_fields {
-            let mut browser_field = if object_path.len() == 1 && object_path[0] == "browser" {
-                if let Some(field) = &data.browser {
-                    field.to_owned()
-                } else {
-                    continue;
+        if let Some(json_object) = raw_json.as_object_mut() {
+            // Remove large fields that are useless for pragmatic use.
+            json_object.remove("description");
+            json_object.remove("keywords");
+            json_object.remove("scripts");
+            json_object.remove("dependencies");
+            json_object.remove("devDependencies");
+            json_object.remove("peerDependencies");
+            json_object.remove("optionalDependencies");
+
+            // Add name.
+            package_json.name =
+                json_object.get("name").and_then(|field| field.as_str()).map(ToString::to_string);
+
+            // Add imports.
+            package_json.imports = json_object
+                .get("imports")
+                .map(ImportExportMap::deserialize)
+                .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());
                 }
-            } else if let Some(field) = Self::get_value_by_path(&data.other_fields, object_path) {
-                BrowserField::deserialize(field)?
-            } else {
-                continue;
-            };
+            }
 
-            // Normalize all relative paths to make browser_field a constant value lookup
-            if let BrowserField::Map(map) = &mut browser_field {
-                let keys = map.keys().cloned().collect::<Vec<_>>();
-                for key in keys {
-                    // Normalize the key if it looks like a file "foo.js"
-                    if key.extension().is_some() {
-                        map.insert(dir.normalize_with(&key), map[&key].clone());
-                    }
-                    // Normalize the key if it is relative path "./relative"
-                    if key.starts_with(".") {
-                        if let Some(value) = map.remove(&key) {
-                            map.insert(dir.normalize_with(&key), value);
+            // Dynamically create `browser_fields`.
+            let dir = path.parent().unwrap();
+            for object_path in &options.alias_fields {
+                if let Some(browser_field) = Self::get_value_by_path(json_object, object_path) {
+                    let mut browser_field = BrowserField::deserialize(browser_field)?;
+
+                    // Normalize all relative paths to make browser_field a constant value lookup
+                    if let BrowserField::Map(map) = &mut browser_field {
+                        let keys = map.keys().cloned().collect::<Vec<_>>();
+                        for key in keys {
+                            // Normalize the key if it looks like a file "foo.js"
+                            if key.extension().is_some() {
+                                map.insert(dir.normalize_with(&key), map[&key].clone());
+                            }
+                            // Normalize the key if it is relative path "./relative"
+                            if key.starts_with(".") {
+                                if let Some(value) = map.remove(&key) {
+                                    map.insert(dir.normalize_with(&key), value);
+                                }
+                            }
                         }
                     }
+                    package_json.browser_fields.push(browser_field);
                 }
             }
-            package_json.browser_fields.push(browser_field);
-        }
 
-        // Dynamically create `exports`.
-        for object_path in &options.exports_fields {
-            if object_path.len() == 1 && object_path[0] == "exports" {
-                if let Some(exports) = &data.exports {
-                    package_json.exports.push(exports.to_owned());
+            // Dynamically create `exports`.
+            for object_path in &options.exports_fields {
+                if let Some(exports) = Self::get_value_by_path(json_object, object_path) {
+                    let exports = ImportExportField::deserialize(exports)?;
+                    package_json.exports.push(exports);
                 }
-            } else if let Some(exports) = Self::get_value_by_path(&data.other_fields, object_path) {
-                package_json.exports.push(ImportExportField::deserialize(exports)?);
             }
         }
 
-        // Bubble up `imports`
-        if let Some(imports) = &data.imports {
-            package_json.imports = Box::new(imports.to_owned());
-        }
-
         package_json.path = path;
         package_json.realpath = realpath;
-
-        // Remove large fields that are useless for pragmatic use
-        if options.remove_unused_fields {
-            data.scripts = None;
-            data.dependencies = None;
-            data.dependencies_meta = None;
-            data.dev_dependencies = None;
-            data.peer_dependencies = None;
-            data.peer_dependencies_meta = None;
-            data.bundle_dependencies = None;
-            data.optional_dependencies = None;
-            data.imports = None;
-            data.exports = None;
-            data.browser = None;
-
-            if let Some(raw_json) = raw_json.as_object_mut() {
-                raw_json.remove("description");
-                raw_json.remove("keywords");
-                raw_json.remove("scripts");
-                raw_json.remove("dependencies");
-                raw_json.remove("devDependencies");
-                raw_json.remove("peerDependencies");
-                raw_json.remove("optionalDependencies");
-            }
-        }
-
-        package_json.data = data;
         package_json.raw_json = Arc::new(raw_json);
-
         Ok(package_json)
     }
 
     fn get_value_by_path<'a>(
-        fields: &'a FxHashMap<String, serde_json::Value>,
+        fields: &'a serde_json::Map<String, serde_json::Value>,
         path: &[String],
     ) -> Option<&'a serde_json::Value> {
         if path.is_empty() {


Test patch:
diff --git a/tests/integration_test.rs b/tests/integration_test.rs
index 2130304..ba8fb82 100644
--- a/tests/integration_test.rs
+++ b/tests/integration_test.rs
@@ -35,11 +35,9 @@ fn eq() {
 #[test]
 fn package_json() {
     let resolution = resolve("./tests/package.json");
-    assert!(resolution.package_json().is_some_and(|json| json
-        .data
-        .name
-        .as_ref()
-        .is_some_and(|name| name == "name")));
+    assert!(resolution
+        .package_json()
+        .is_some_and(|json| json.name.as_ref().is_some_and(|name| name == "name")));
 }
 
 #[test]