Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: normalize resolved result on Windows #322

Conversation

sapphi-red
Copy link
Contributor

#316 seems to break Windows.

If the specifier or the base directory contains a forward slash, the resolved result contained a forward slash.
This causes the same module to be resolved differently (e.g. C:\\foo.js and C:/foo.js) and ends up bundling the same module twice.

For example, import './foo.js'; import '.\\foo.js' should resolve to the same path.

To fix this, without reverting #316, I think it requires thinking about where to normalize the slashes. But it takes some time for me to do that, so in this PR I simply reverted the change.

reverts #316

@@ -44,7 +44,7 @@ impl<Fs: FileSystem> Cache<Fs> {
pub fn value(&self, path: &Path) -> CachedPath {
let hash = {
let mut hasher = FxHasher::default();
path.as_os_str().hash(&mut hasher);
path.hash(&mut hasher);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
path.hash(&mut hasher);
#[cfg(windows)]
path.hash(&mut hasher);
#[cfg(not(windows))]
path.as_os_str().hash(&mut hasher);

It might be possible to do this. But I'm not sure that's safe for all non-Windows OSs.

Copy link

codspeed-hq bot commented Nov 25, 2024

CodSpeed Performance Report

Merging #322 will degrade performances by 17.72%

Comparing sapphi-red:fix/normalize-resolved-result-on-windows (8000aeb) with main (290c972)

Summary

❌ 3 regressions

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main sapphi-red:fix/normalize-resolved-result-on-windows Change
resolver[multi-thread] 461.7 µs 516.1 µs -10.54%
resolver[resolve from symlinks] 51.3 ms 62.4 ms -17.72%
resolver[single-thread] 419.4 µs 482.3 µs -13.04%

@sapphi-red
Copy link
Contributor Author

Ah, it seems the test passes with this diff + 08a19d7, but it doesn't with the main branch.

@Boshen
Copy link
Member

Boshen commented Nov 25, 2024

I wonder whether we should force upstream to normalize all paths ... let me think about this.

Or, change to a simpler, customized hasher.

@sapphi-red
Copy link
Contributor Author

I thought it would work with a simple revert, but it turns out it needs a bigger change than that 😅.

@sapphi-red
Copy link
Contributor Author

I'll leave this one for now.

@Boshen Boshen self-assigned this Nov 25, 2024
@Boshen
Copy link
Member

Boshen commented Nov 25, 2024

I'll leave this one for now.

No worries, I'll take over.

Boshen added a commit that referenced this pull request Dec 5, 2024
Boshen added a commit that referenced this pull request Dec 5, 2024
Boshen added a commit that referenced this pull request Dec 5, 2024
@Boshen Boshen closed this in #328 Dec 5, 2024
@Boshen Boshen closed this in 790332c Dec 5, 2024
Boshen pushed a commit that referenced this pull request Dec 13, 2024
@sapphi-red sapphi-red deleted the fix/normalize-resolved-result-on-windows branch December 13, 2024 09:23
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants