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.
This commit is contained in:
Boshen 2023-07-23 23:41:27 +08:00 committed by GitHub
parent 65f22f9ba1
commit 8ab2a7f322
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 61 additions and 61 deletions

View file

@ -165,7 +165,6 @@ impl<Fs: FileSystem> ResolverGeneric<Fs> {
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<Fs: FileSystem> ResolverGeneric<Fs> {
}
// 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<Fs: FileSystem> ResolverGeneric<Fs> {
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<Fs: FileSystem> ResolverGeneric<Fs> {
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<Fs: FileSystem> ResolverGeneric<Fs> {
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<Fs: FileSystem> ResolverGeneric<Fs> {
/// * 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<Fs: FileSystem> ResolverGeneric<Fs> {
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?

View file

@ -20,12 +20,27 @@ pub struct PackageJson {
#[serde(untagged)]
pub enum BrowserField {
String(String),
Map(HashMap<String, serde_json::Value>),
Map(HashMap<PathBuf, serde_json::Value>),
}
impl PackageJson {
pub fn parse(path: PathBuf, json: &str) -> Result<Self, serde_json::Error> {
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::<Vec<_>>();
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<Option<&str>, 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 <https://github.com/defunctzombie/package-browser-field-spec#alternate-main---basic>
_ => 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<Option<&str>, 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<Option<&'a str>, 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)
}
}