Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix: 🐛 edge case one package cached and unplugged in same time #5

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

stormslowly
Copy link
Contributor

@stormslowly stormslowly commented Feb 10, 2025

problem

in this rspack issue's reproduction pnp-rs resolves successfully by chance

reproduce steps

  1. clone git cloen [email protected]:kubijo/rspack-link-repro.git
  2. try several time in pnp-rs cargo run -- @carbon/icon-helpers /path/to/rspack-link-repro/.yarn/unplugged/@carbon-icons-react-virtual-379302d360/node_modules/@carbon/icons-react/es/
    if we get luck, it will shows
specifier = @carbon/icon-helpers
parent    = "/path/to/rspack-link-repro/.yarn/unplugged/@carbon-icons-react-virtual-379302d360/node_modules/@carbon/icons-react/es/"
@carbon/icons-react tried to access @carbon/icon-helpers, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound.

root cause

in this case https://github.com/kubijo/rspack-link-repro/blob/70339cbc0911432f1a5dfdbf497f1e7b44606c36/.pnp.cjs#L2104-L2128
pkg @carbon/icons-react has both cached and unplugged dir, so it have two package infos, in the state json

then manifest.location_trie.insert depends on the iteration order of ranges

pnp-rs/src/lib.rs

Lines 263 to 278 in cbfa5be

for (reference, info) in ranges.iter_mut() {
let package_location = manifest.manifest_dir
.join(info.package_location.clone());
let normalized_location = arca::path::normalize_path(
&package_location.to_string_lossy(),
);
info.package_location = PathBuf::from(normalized_location);
if !info.discard_from_lookup {
manifest.location_trie.insert(&info.package_location, PackageLocator {
name: name.clone(),
reference: reference.clone(),
});
}

if @carbon/icons-react's virtual:ed977161de6 package info comes last, resolve success; if npm:11.54.0 comes last, resolve failed.

so we need to keep the package info's order as them are in json state.

solution

Just use IndexMap instead of Hashmap.

@arcanis
Copy link
Member

arcanis commented Feb 10, 2025

Interesting edge case! The fix looks fine by me - do you know how we could check the perf impact, just to make sure it's reasonable? If too heavy, a lighter alternative would be to prune packages that have virtual counterparts 🤔

@stormslowly
Copy link
Contributor Author

@arcanis i made a benchmark testing in rspack-resolver
it this benchmark test, 400 npm packages are installed with pnp mode, then resolve these packages one by one.

from rspack-resolver is view , this PR does't impact on resolving performance

using Hashmap

resolver/resolve in large pnp project
time: [2.6017 ms 2.7252 ms 2.8860 ms]
change: [−47.009% −5.8115% +61.486%] (p = 0.85 > 0.05)
No change in performance detected.

using IndexMap

resolver/resolve in large pnp project
time: [2.7101 ms 2.8460 ms 3.0212 ms]
change: [−40.699% +6.5169% +89.302%] (p = 0.85 > 0.05)
No change in performance detected.

@arcanis arcanis merged commit 33b1723 into yarnpkg:main Feb 17, 2025
1 check passed
@arcanis
Copy link
Member

arcanis commented Feb 17, 2025

Thanks! I'll make a release later today and make a PR to upgrade the dependency on rspack_resolve

@stormslowly stormslowly deleted the fix/cached_unplugged branch February 19, 2025 06:30
@stormslowly
Copy link
Contributor Author

stormslowly commented Feb 19, 2025

Thanks! I'll make a release later today and make a PR to upgrade the dependency on rspack_resolve

@arcanis when will you publish pnp-rs's new version; there is a issue needs this PR.

@arcanis
Copy link
Member

arcanis commented Feb 21, 2025

Released as 0.9.1!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants