Skip to content

Commit ea425f6

Browse files
committed
perf: do not canonicalize the entry path (#848)
This was introduced in `https://github.com/unrs/unrs-resolver/pull/125` The PR description and test case do not explain the reasoning behind this change, and I don't see how it reflects a real-world scenario in modern package managers.
1 parent 8e826e7 commit ea425f6

File tree

8 files changed

+8
-35
lines changed

8 files changed

+8
-35
lines changed

fixtures/symlink-with-nested-node_modules/.gitignore

Lines changed: 0 additions & 1 deletion
This file was deleted.

fixtures/symlink-with-nested-node_modules/bar/node_modules/foo

Lines changed: 0 additions & 1 deletion
This file was deleted.

fixtures/symlink-with-nested-node_modules/foo/node_modules/dep/index.js

Whitespace-only changes.

fixtures/symlink-with-nested-node_modules/foo/node_modules/foo/index.js

Whitespace-only changes.

src/lib.rs

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -260,20 +260,9 @@ impl<Fs: FileSystem> ResolverGeneric<Fs> {
260260
) -> Result<Resolution, ResolveError> {
261261
ctx.with_fully_specified(self.options.fully_specified);
262262

263-
let cached_path = if self.options.symlinks {
264-
self.load_realpath(&self.cache.value(path))?
265-
} else {
266-
path.to_path_buf()
267-
};
268-
269-
let cached_path = self.cache.value(&cached_path);
263+
let cached_path = self.cache.value(path);
270264
let cached_path = self.require(&cached_path, specifier, ctx)?;
271-
272-
let path = if self.options.symlinks {
273-
self.load_realpath(&cached_path)?
274-
} else {
275-
cached_path.to_path_buf()
276-
};
265+
let path = self.load_realpath(&cached_path)?;
277266

278267
let package_json = self.find_package_json_for_a_package(&cached_path, ctx)?;
279268
if let Some(package_json) = &package_json {

src/tests/package_json.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
//! Tests for `Resolution::package_json`.
22
3-
use crate::{ResolveError, Resolver};
3+
use crate::Resolver;
44

55
#[test]
66
fn test() {
@@ -68,8 +68,9 @@ fn package_json_with_symlinks_true() {
6868
fn test_corrupted_package_json() {
6969
use std::path::Path;
7070

71+
use crate::{ResolveError, ResolveOptions, ResolverGeneric};
72+
7173
use super::memory_fs::MemoryFS;
72-
use crate::{ResolveOptions, ResolverGeneric};
7374

7475
// Test scenarios for various corrupted package.json files
7576
let scenarios = [

src/tests/resolve.rs

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -165,22 +165,6 @@ fn resolve_dot() {
165165
}
166166
}
167167

168-
#[test]
169-
fn symlink_with_nested_node_modules() {
170-
let f = super::fixture_root().join("symlink-with-nested-node_modules");
171-
172-
let resolver = Resolver::default();
173-
let resolved_path =
174-
resolver.resolve(f.join("bar/node_modules/foo"), "dep").map(|r| r.full_path());
175-
assert_eq!(resolved_path, Ok(f.join("foo/node_modules/dep/index.js")));
176-
177-
let resolver = Resolver::new(ResolveOptions { symlinks: false, ..ResolveOptions::default() });
178-
assert_eq!(
179-
resolver.resolve(f.join("bar/node_modules/foo"), "dep"),
180-
Err(ResolveError::NotFound("dep".into()))
181-
);
182-
}
183-
184168
#[test]
185169
fn abnormal_relative() {
186170
let f = super::fixture_root().join("abnormal-relative-with-node_modules");

src/tests/symlink.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ fn test() {
174174
fn test_unsupported_targets() {
175175
use crate::ResolveError;
176176

177-
let Some(SymlinkFixturePaths { root: _, temp_path }) =
177+
let Some(SymlinkFixturePaths { root, temp_path }) =
178178
prepare_symlinks("temp.test_unsupported_targets").unwrap()
179179
else {
180180
return;
@@ -200,9 +200,10 @@ fn test_unsupported_targets() {
200200
// from `FsCachedPath::find_package_json` when trying to canonicalize the full path of `package.json`.
201201
// * Otherwise, a `ResolveError::NotFound` will be returned.
202202
let dos_device_temp_path = get_dos_device_path(&temp_path).unwrap();
203+
let dos_device_root = get_dos_device_path(&root).unwrap();
203204
assert_eq!(
204205
resolver_with_symlinks.resolve(&dos_device_temp_path, "./index.js"),
205-
Err(ResolveError::PathNotSupported(dos_device_temp_path))
206+
Err(ResolveError::PathNotSupported(dos_device_root))
206207
);
207208
}
208209

0 commit comments

Comments
 (0)