-
-
Notifications
You must be signed in to change notification settings - Fork 23
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
Implements PnP support #217
Conversation
I'll start experimenting with this feature locally by initiating a yarn pnp project. |
fixtures/pnp/.yarn/cache/react-dom-npm-18.3.1-a805663f38-a752496c19.zip
Outdated
Show resolved
Hide resolved
Things left for me to do:
|
The tests should work, I may have broken something before push; checking it now (my intent is that the presence or not of the PnP manifest is the feature flag - it should have 0 impact on people that aren't using PnP) |
The latest changes is working on my end: With a new project: {
"name": "test-yarn",
"packageManager": "[email protected]",
"dependencies": {
"lodash": "^4.17.21"
}
} Update the options in let options = ResolveOptions {
pnp_manifest: pnp::load_pnp_manifest("/Users/boshen/github/test-yarn/.pnp.cjs").ok(),
..ResolveOptions::default()
}; cargo run --example resolver -- /Users/boshen/github/test-yarn lodash prints
|
CodSpeed Performance ReportMerging #217 will not alter performanceComparing Summary
|
I understood that the option Unfortunately Cargo will pull in and compile all the unused dependencies. We need to a feature flag to avoid these extra dependencies at compile time. |
Thank you arcanis, this integration is really smooth. I'll take care of the rest. Will ping you or create an issue in |
That's unfortunate :( Would it help if we had fewer dependencies? My concern is that every tool using oxc-resolver would in theory benefit from having PnP support; if they have to do something to get it to work, they may forget doing it (the necessity of manually passing the |
The tool will decide whether they want to enable pnp. All the Rust bundlers will enable pnp. Simple tools that only require simple resolution will not enable it.
I'll take a look at how enhanced-resolve and parcel handles this problem. |
Enhanced-resolve checks whether the process is running within the PnP runtime, so no configuration is needed. Parcel doesn't support PnP yet (I only prototyped it a year ago, but didn't open the PR). |
Thank you for the context, I have gathered the following information:
For bundlers with napi integration, they can get the manifest path in node.js and pass it over. So the question is, are we able to get the manifest path from For tools without a node.js runtime, they'll have to provide the path explicitly as a compromise. |
Yes, there are two ways:
|
It would be worth looking into what esbuild does for detecting PnP manifest since using |
I'm going to merge this and continue working on the main branch. Thank you arcanis for the hard work. |
(note: each commit contains a particular semantic change, so you can review them one by one if you prefer)
This PR implements support for PnP. I opened it in draft since I'm sure some feedback rounds will be required, but it should already be working fine (added some tests to verify that).
There are two main changes:
During the
node_modules
resolution we first check whether a PnP manifest is attached to the resolver. If it is we query the PnP resolver to locate the package directory instead of performing the typicalnode_modules
traversal. This directory is then used in oxc's subsequent resolution passes (exports
, extensions, etc).Since Yarn stores the packages inside a zipped cache (details here), oxc needs to know how to look into them. The
pnp
crate provides some utilities to make this work simpler, which I used here. It adds some boilerplate, I'm open to ideas to clean it up (but since it's in fairly low amount code in absolute measures, perhaps it's ok?).There's one known limitation: PnP technically supports interlacing completely different dependency trees. This only happens when a project with a
yarn.lock
tries to import files from another project with a differentyarn.lock
, which is very rare (it usually only ever come up when using some specific scenarios ofyarn dlx
). I didn't implement it here, the manifest is instead passed as option (but perhaps we should instead locate the manifest by crawling the filesystem?).Fixes #53