From 8ab2a7f322397d3da01f7f355c10c28d1a2f9f5c Mon Sep 17 00:00:00 2001 From: Boshen Date: Sun, 23 Jul 2023 23:41:27 +0800 Subject: [PATCH] refactor(resolver): improve browser_field lookup (#592) This makes everything slow because every file read now needs to check package.json for the browser field, but we'll improve the package.json look up soon by reusing the values from the cache. --- crates/oxc_resolver/src/lib.rs | 52 +++++++++--------- crates/oxc_resolver/src/package_json.rs | 70 ++++++++++++------------- 2 files changed, 61 insertions(+), 61 deletions(-) diff --git a/crates/oxc_resolver/src/lib.rs b/crates/oxc_resolver/src/lib.rs index 29086b7f7..7e58b7ca9 100644 --- a/crates/oxc_resolver/src/lib.rs +++ b/crates/oxc_resolver/src/lib.rs @@ -165,7 +165,6 @@ impl ResolverGeneric { Err(ResolveError::NotFound(path.to_path_buf().into_boxed_path())) } - #[allow(clippy::unnecessary_wraps)] fn load_as_file(&self, path: &Path) -> ResolveState { // enhanced-resolve feature: extension_alias if let Some(path) = self.load_extension_alias(path)? { @@ -173,16 +172,16 @@ impl ResolverGeneric { } // 1. If X is a file, load X as its file extension format. STOP - if self.cache.is_file(path) { - return Ok(Some(path.to_path_buf())); + if let Some(path) = self.load_alias_or_file(path)? { + return Ok(Some(path)); } // 2. If X.js is a file, load X.js as JavaScript text. STOP // 3. If X.json is a file, parse X.json to a JavaScript Object. STOP // 4. If X.node is a file, load X.node as binary addon. STOP for extension in &self.options.extensions { let path_with_extension = path.with_extension(extension); - if self.cache.is_file(&path_with_extension) { - return Ok(Some(path_with_extension)); + if let Some(path) = self.load_alias_or_file(&path_with_extension)? { + return Ok(Some(path)); } } Ok(None) @@ -192,32 +191,39 @@ impl ResolverGeneric { if self.options.symlinks { self.cache.canonicalize(path) } else { None } } - #[allow(clippy::unnecessary_wraps)] - fn load_index(&self, path: &Path, package_json: Option<&PackageJson>) -> ResolveState { + fn load_index(&self, path: &Path) -> ResolveState { for main_field in &self.options.main_files { - if let Some(package_json) = package_json { - if let Some(path) = self.load_browser_field(path, main_field, package_json)? { + let main_path = path.join(main_field); + if self.options.enforce_extension == Some(false) { + if let Some(path) = self.load_alias_or_file(&main_path)? { return Ok(Some(path)); } } - - let main_path = path.join(main_field); - if self.options.enforce_extension == Some(false) && self.cache.is_file(&main_path) { - return Ok(Some(main_path)); - } // 1. If X/index.js is a file, load X/index.js as JavaScript text. STOP // 2. If X/index.json is a file, parse X/index.json to a JavaScript object. STOP // 3. If X/index.node is a file, load X/index.node as binary addon. STOP for extension in &self.options.extensions { let main_path_with_extension = main_path.with_extension(extension); - if self.cache.is_file(&main_path_with_extension) { - return Ok(Some(main_path_with_extension)); + if let Some(path) = self.load_alias_or_file(&main_path_with_extension)? { + return Ok(Some(path)); } } } Ok(None) } + fn load_alias_or_file(&self, path: &Path) -> ResolveState { + if let Some(package_json) = self.cache.find_package_json(path)? { + if let Some(path) = self.load_browser_field(path, None, &package_json)? { + return Ok(Some(path)); + } + } + if self.cache.is_file(path) { + return Ok(Some(path.to_path_buf())); + } + Ok(None) + } + fn load_as_directory(&self, path: &Path) -> ResolveState { // TODO: Only package.json is supported, so warn about having other values // Checking for empty files is needed for omitting checks on package.json @@ -234,9 +240,7 @@ impl ResolverGeneric { return Ok(Some(path)); } // e. LOAD_INDEX(M) - if let Some(path) = - self.load_index(&main_field_path, Some(package_json.as_ref()))? - { + if let Some(path) = self.load_index(&main_field_path)? { return Ok(Some(path)); } // f. LOAD_INDEX(X) DEPRECATED @@ -244,13 +248,13 @@ impl ResolverGeneric { return Err(ResolveError::NotFound(main_field_path.into_boxed_path())); } - if let Some(path) = self.load_index(path, Some(package_json.as_ref()))? { + if let Some(path) = self.load_index(path)? { return Ok(Some(path)); } } } // 2. LOAD_INDEX(X) - self.load_index(path, None) + self.load_index(path) } fn load_node_modules(&self, start: &Path, request: &str) -> ResolveState { @@ -304,7 +308,7 @@ impl ResolverGeneric { /// * Parent of package.json is None fn load_package_self(&self, path: &Path, request: &str) -> ResolveState { if let Some(package_json) = self.cache.find_package_json(path)? { - if let Some(path) = self.load_browser_field(path, request, &package_json)? { + if let Some(path) = self.load_browser_field(path, Some(request), &package_json)? { return Ok(Some(path)); } } @@ -314,10 +318,10 @@ impl ResolverGeneric { fn load_browser_field( &self, path: &Path, - request: &str, + request: Option<&str>, package_json: &PackageJson, ) -> ResolveState { - if let Some(request) = package_json.resolve(path, request, &self.options.extensions)? { + if let Some(request) = package_json.resolve(path, request)? { let request = Request::parse(request).map_err(ResolveError::Request)?; debug_assert!(package_json.path.file_name().is_some_and(|x| x == "package.json")); // TODO: Do we need to pass query and fragment? diff --git a/crates/oxc_resolver/src/package_json.rs b/crates/oxc_resolver/src/package_json.rs index 54be555bb..8a3f4ce69 100644 --- a/crates/oxc_resolver/src/package_json.rs +++ b/crates/oxc_resolver/src/package_json.rs @@ -20,12 +20,27 @@ pub struct PackageJson { #[serde(untagged)] pub enum BrowserField { String(String), - Map(HashMap), + Map(HashMap), } impl PackageJson { pub fn parse(path: PathBuf, json: &str) -> Result { let mut package_json: Self = serde_json::from_str(json)?; + + // Normalize all relative paths to make browser_field a constant value lookup + // TODO: fix BrowserField::String + if let Some(BrowserField::Map(map)) = &mut package_json.browser { + let relative_paths = + map.keys().filter(|path| path.starts_with(".")).cloned().collect::>(); + let dir = path.parent().unwrap(); + for relative_path in relative_paths { + if let Some(value) = map.remove(&relative_path) { + let normalized_path = dir.normalize_with(relative_path); + map.insert(normalized_path, value); + } + } + } + package_json.path = path; Ok(package_json) } @@ -38,53 +53,34 @@ impl PackageJson { pub fn resolve( &self, path: &Path, - request: &str, - extensions: &[String], + request: Option<&str>, ) -> Result, ResolveError> { // TODO: return ResolveError if the provided `alias_fields` is not `browser` for future proof match self.browser.as_ref() { - Some(BrowserField::Map(map)) => { - for (key, value) in map { - if let Some(resolved_str) = - self.resolve_browser_field(path, key, value, request, extensions)? - { - return Ok(Some(resolved_str)); - } - } - Ok(None) + Some(BrowserField::Map(field_data)) => { + // look up by full path if request is empty + request + .map_or_else( + || field_data.get(path), + |request| field_data.get(Path::new(request)), + ) + .map_or_else(|| Ok(None), |value| Self::alias_value(path, value)) } // TODO: implement _ => Ok(None), } } - // TODO: refactor this mess - fn resolve_browser_field<'a>( - &'a self, - start: &Path, - key: &str, + fn alias_value<'a>( + key: &Path, value: &'a serde_json::Value, - request: &str, - extensions: &[String], - ) -> Result, ResolveError> { - let directory = self.path.parent().unwrap(); // `unwrap`: this is a path to package.json, parent is its containing directory - let right = directory.join(key).normalize(); - let left = start.join(request).normalize(); - if key == request - || extensions.iter().any(|ext| Path::new(request).with_extension(ext) == Path::new(key)) - || right == left - || extensions.iter().any(|ext| left.with_extension(ext) == right) - { - if let serde_json::Value::String(value) = value { - return Ok(Some(value.as_str())); + ) -> Result, ResolveError> { + match value { + serde_json::Value::String(value) => Ok(Some(value.as_str())), + serde_json::Value::Bool(b) if !b => { + Err(ResolveError::Ignored(key.to_path_buf().into_boxed_path())) } - - // key match without string value, i.e. `"path": false` for ignore - let directory = self.path.parent().unwrap(); // `unwrap`: this is a path to package.json, parent is its containing directory - let path_key = directory.join(key).normalize(); - return Err(ResolveError::Ignored(path_key.into_boxed_path())); + _ => Ok(None), } - - Ok(None) } }