Skip to content

Commit

Permalink
fix: change ResolveError::NotFound(PathBuf) to report specifier
Browse files Browse the repository at this point in the history
  • Loading branch information
Boshen committed Jan 3, 2024
1 parent 0fdbc75 commit ae0dbac
Show file tree
Hide file tree
Showing 11 changed files with 48 additions and 33 deletions.
6 changes: 3 additions & 3 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ pub enum ResolveError {
#[error("Path is ignored {0}")]
Ignored(PathBuf),

/// Path not found
#[error("Path not found {0}")]
NotFound(PathBuf),
/// Module not found
#[error("Cannot find module '{0}'")]
NotFound(/* specifier */ String),

/// Tsconfig not found
#[error("Tsconfig not found {0}")]
Expand Down
32 changes: 17 additions & 15 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {
if let Some(path) = self.load_as_file_or_directory(&path, specifier, ctx)? {
return Ok(path);
}
Err(ResolveError::NotFound(cached_path.to_path_buf()))
Err(ResolveError::NotFound(specifier.to_string()))
} else {
for root in &self.options.roots {
let cached_path = self.cache.value(root);
Expand All @@ -260,7 +260,7 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {
return Ok(path);
}
}
Err(ResolveError::NotFound(cached_path.to_path_buf()))
Err(ResolveError::NotFound(specifier.to_string()))
}
}

Expand All @@ -279,7 +279,7 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {
return Ok(path);
}
// c. THROW "not found"
Err(ResolveError::NotFound(path))
Err(ResolveError::NotFound(specifier.to_string()))
}

fn require_hash(
Expand Down Expand Up @@ -353,7 +353,7 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {
return Ok(path);
}
// 7. THROW "not found"
Err(ResolveError::NotFound(cached_path.to_path_buf()))
Err(ResolveError::NotFound(specifier.to_string()))
}

/// LOAD_PACKAGE_IMPORTS(X, DIR)
Expand All @@ -374,9 +374,9 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {
return Ok(None);
}
// 4. let MATCH = PACKAGE_IMPORTS_RESOLVE(X, pathToFileURL(SCOPE), ["node", "require"]) defined in the ESM resolver.
let path = self.package_imports_resolve(&package_json, specifier, ctx)?;
let path = self.package_imports_resolve(specifier, &package_json, ctx)?;
// 5. RESOLVE_ESM_MATCH(MATCH).
self.resolve_esm_match(&path, &package_json, ctx)
self.resolve_esm_match(specifier, &path, &package_json, ctx)
}

fn load_as_file(&self, cached_path: &CachedPath, ctx: &mut Ctx) -> ResolveResult {
Expand Down Expand Up @@ -571,7 +571,9 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {
// Try foo/node_modules/package_name
if cached_path.is_dir(&self.cache.fs) {
// a. LOAD_PACKAGE_EXPORTS(X, DIR)
if let Some(path) = self.load_package_exports(subpath, &cached_path, ctx)? {
if let Some(path) =
self.load_package_exports(specifier, subpath, &cached_path, ctx)?
{
return Ok(Some(path));
}
} else {
Expand Down Expand Up @@ -620,6 +622,7 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {

fn load_package_exports(
&self,
specifier: &str,
subpath: &str,
cached_path: &CachedPath,
ctx: &mut Ctx,
Expand All @@ -646,7 +649,7 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {
ctx,
)? {
// 6. RESOLVE_ESM_MATCH(MATCH)
return self.resolve_esm_match(&path, &package_json, ctx);
return self.resolve_esm_match(specifier, &path, &package_json, ctx);
};
}
Ok(None)
Expand Down Expand Up @@ -691,7 +694,7 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {
ctx,
)? {
// 6. RESOLVE_ESM_MATCH(MATCH)
return self.resolve_esm_match(&cached_path, &package_json, ctx);
return self.resolve_esm_match(specifier, &cached_path, &package_json, ctx);
}
}
Ok(None)
Expand All @@ -700,24 +703,23 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {
/// RESOLVE_ESM_MATCH(MATCH)
fn resolve_esm_match(
&self,
specifier: &str,
cached_path: &CachedPath,
package_json: &PackageJson,
ctx: &mut Ctx,
) -> ResolveResult {
if let Some(path) = self.load_browser_field(cached_path, None, package_json, ctx)? {
return Ok(Some(path));
}

// 1. let RESOLVED_PATH = fileURLToPath(MATCH)
// 2. If the file at RESOLVED_PATH exists, load RESOLVED_PATH as its extension format. STOP
//
// Non-compliant ESM can result in a directory, so directory is tried as well.
if let Some(path) = self.load_as_file_or_directory(cached_path, "", ctx)? {
return Ok(Some(path));
}

// 3. THROW "not found"
Err(ResolveError::NotFound(cached_path.to_path_buf()))
Err(ResolveError::NotFound(specifier.to_string()))
}

/// enhanced-resolve: AliasFieldPlugin for [ResolveOptions::alias_fields]
Expand All @@ -743,7 +745,7 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {
return if cached_path.is_file(&self.cache.fs) {
Ok(Some(cached_path.clone()))
} else {
Err(ResolveError::NotFound(cached_path.path().to_path_buf()))
Err(ResolveError::NotFound(new_specifier.to_string()))
};
}
return Err(ResolveError::Recursion);
Expand Down Expand Up @@ -1015,7 +1017,7 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {
}
}

Err(ResolveError::NotFound(cached_path.to_path_buf()))
Err(ResolveError::NotFound(specifier.to_string()))
}

/// PACKAGE_EXPORTS_RESOLVE(packageURL, subpath, exports, conditions)
Expand Down Expand Up @@ -1124,8 +1126,8 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {
/// PACKAGE_IMPORTS_RESOLVE(specifier, parentURL, conditions)
fn package_imports_resolve(
&self,
package_json: &PackageJson,
specifier: &str,
package_json: &PackageJson,
ctx: &mut Ctx,
) -> Result<CachedPath, ResolveError> {
// 1. Assert: specifier begins with "#".
Expand Down
6 changes: 3 additions & 3 deletions src/tests/browser_field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,9 @@ fn recurse_fail() {

#[rustfmt::skip]
let data = [
("recurse non existent", f.clone(), "./lib/non-existent.js", ResolveError::NotFound(f.join("lib/non-existent.js"))),
("path partial match 1", f.clone(), "./xyz.js", ResolveError::NotFound(f.join("xyz.js"))),
("path partial match 2", f.clone(), "./lib/xyz.js", ResolveError::NotFound(f.join("lib/xyz.js"))),
("recurse non existent", f.clone(), "./lib/non-existent.js", ResolveError::NotFound("./lib/non-existent.js".into())),
("path partial match 1", f.clone(), "./xyz.js", ResolveError::NotFound("./xyz.js".into())),
("path partial match 2", f.clone(), "./lib/xyz.js", ResolveError::NotFound("./lib/xyz.js".into())),
];

for (comment, path, request, expected) in data {
Expand Down
4 changes: 2 additions & 2 deletions src/tests/builtins.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::path::{Path, PathBuf};
use std::path::Path;

use crate::{ResolveError, ResolveOptions, Resolver};

Expand All @@ -7,7 +7,7 @@ fn builtins_off() {
let f = Path::new("/");
let resolver = Resolver::default();
let resolved_path = resolver.resolve(f, "zlib").map(|r| r.full_path());
assert_eq!(resolved_path, Err(ResolveError::NotFound(PathBuf::from("/"))));
assert_eq!(resolved_path, Err(ResolveError::NotFound("zlib".into())));
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion src/tests/exports_field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ fn test() {
// ("throw error if extension not provided", f2.clone(), "exports-field/dist/main", ResolveError::NotFound(f2.join("node_modules/exports-field/lib/lib2/main"))),
("resolver should respect query parameters #2. Direct matching", f2.clone(), "exports-field?foo", ResolveError::PackagePathNotExported("./?foo".into(), p2.clone())),
("resolver should respect fragment parameters #2. Direct matching", f2, "exports-field#foo", ResolveError::PackagePathNotExported("./#foo".into(), p2.clone())),
("relative path should not work with exports field", f.clone(), "./node_modules/exports-field/dist/main.js", ResolveError::NotFound(f.join("node_modules/exports-field/dist/main.js"))),
("relative path should not work with exports field", f.clone(), "./node_modules/exports-field/dist/main.js", ResolveError::NotFound("./node_modules/exports-field/dist/main.js".into())),
("backtracking should not work for request", f.clone(), "exports-field/dist/../../../a.js", ResolveError::InvalidPackageTarget("./lib/../../../a.js".to_string(), "/dist/".to_string(), p.clone())),
("backtracking should not work for exports field target", f.clone(), "exports-field/dist/a.js", ResolveError::InvalidPackageTarget("./../../a.js".to_string(), "/dist/a.js".to_string(), p.clone())),
("not exported error", f.clone(), "exports-field/anything/else", ResolveError::PackagePathNotExported("./anything/else".to_string(), p.clone())),
Expand Down
4 changes: 2 additions & 2 deletions src/tests/extensions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ fn extensions() {

#[rustfmt::skip]
let fail = [
("not resolve to file when request has a trailing slash (relative)", "./foo.js/", f.join("foo.js"))
("not resolve to file when request has a trailing slash (relative)", "./foo.js/", "./foo.js/".into())
];

for (comment, request, expected_error) in fail {
Expand Down Expand Up @@ -93,7 +93,7 @@ fn multi_dot_extension() {

#[rustfmt::skip]
let fail = [
("not resolve to file", "./index.", f.join("index."))
("not resolve to file", "./index.", "./index.".into())
];

for (comment, request, expected_error) in fail {
Expand Down
2 changes: 1 addition & 1 deletion src/tests/incorrect_description_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,5 +55,5 @@ fn no_description_file() {
// without description file
let resolver =
Resolver::new(ResolveOptions { description_files: vec![], ..ResolveOptions::default() });
assert_eq!(resolver.resolve(&f, "."), Err(ResolveError::NotFound(f)));
assert_eq!(resolver.resolve(&f, "."), Err(ResolveError::NotFound(".".into())));
}
10 changes: 9 additions & 1 deletion src/tests/resolve.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! <https://github.com/webpack/enhanced-resolve/blob/main/test/resolve.test.js>
use crate::{ResolveOptions, Resolver};
use crate::{ResolveError, ResolveOptions, Resolver};

#[test]
fn resolve() {
Expand Down Expand Up @@ -109,3 +109,11 @@ fn resolve_to_context() {
assert_eq!(resolved_path, Ok(expected), "{comment} {path:?} {request}");
}
}

#[test]
fn resolve_hash_as_module() {
let f = super::fixture();
let resolver = Resolver::new(ResolveOptions::default());
let resolution = resolver.resolve(&f, "#a");
assert_eq!(resolution, Err(ResolveError::NotFound("#a".into())));
}
2 changes: 1 addition & 1 deletion src/tests/roots.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ fn roots() {

#[rustfmt::skip]
let fail = [
("should not work with relative path", "fixtures/b.js", ResolveError::NotFound(f.clone()))
("should not work with relative path", "fixtures/b.js", ResolveError::NotFound("fixtures/b.js".into()))
];

for (comment, request, expected) in fail {
Expand Down
7 changes: 6 additions & 1 deletion src/tests/tsconfig_paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,11 @@ OneTest {
for test in fail {
let resolved_path =
test.resolver(&root).resolve(&root, test.requested_module).map(|f| f.full_path());
assert_eq!(resolved_path, Err(ResolveError::NotFound("/root".into())), "{}", test.name);
assert_eq!(
resolved_path,
Err(ResolveError::NotFound(test.requested_module.into())),
"{}",
test.name
);
}
}
6 changes: 3 additions & 3 deletions src/tests/tsconfig_project_references.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ fn disabled() {
(f.join("app"), "@/index.ts", Ok(f.join("app/aliased/index.ts"))),
(f.join("app"), "@/../index.ts", Ok(f.join("app/index.ts"))),
// Test project reference
(f.join("project_a"), "@/index.ts", Err(ResolveError::NotFound(f.join("project_a")))),
(f.join("project_b/src"), "@/index.ts", Err(ResolveError::NotFound(f.join("project_b/src")))),
(f.join("project_a"), "@/index.ts", Err(ResolveError::NotFound("@/index.ts".into()))),
(f.join("project_b/src"), "@/index.ts", Err(ResolveError::NotFound("@/index.ts".into()))),
// Does not have paths alias
(f.join("project_a"), "./index.ts", Ok(f.join("project_a/index.ts"))),
(f.join("project_c"), "./index.ts", Ok(f.join("project_c/index.ts"))),
Expand Down Expand Up @@ -83,7 +83,7 @@ fn manual() {
(f.join("app"), "@/../index.ts", Ok(f.join("app/index.ts"))),
// Test project reference
(f.join("project_a"), "@/index.ts", Ok(f.join("project_a/aliased/index.ts"))),
(f.join("project_b/src"), "@/index.ts", Err(ResolveError::NotFound(f.join("project_b/src")))),
(f.join("project_b/src"), "@/index.ts", Err(ResolveError::NotFound("@/index.ts".into()))),
// Does not have paths alias
(f.join("project_a"), "./index.ts", Ok(f.join("project_a/index.ts"))),
(f.join("project_c"), "./index.ts", Ok(f.join("project_c/index.ts"))),
Expand Down

0 comments on commit ae0dbac

Please sign in to comment.