feat(core): add yarn pnp support #7639
Conversation
github-actions
bot
added
the
team
✅ Deploy Preview for rspack canceled.
|
github-actions
bot
added
the
release: feature
|
!bench |
|
⏳ Triggered benchmark: Open |
| @@ -9822,10 +9822,12 @@ type ResolveRequest = BaseResolveRequest & Partial<ParsedIdentifier>; | |||
|
|
|||
| // @public (undocumented) | |||
| class ResolverFactory { | |||
There was a problem hiding this comment.
@h-a-n-a is this real public api? how users can access this api?
There was a problem hiding this comment.
This is not according to the generated api.md
| ) -> napi::Result<JsResolver> { | ||
| let pnp_manifest_path = find_closest_pnp_manifest_path(context); |
There was a problem hiding this comment.
This API seems to have a recursive I/O operation. Is it possible to make it opt-in?
There was a problem hiding this comment.
then we may need feature flag in rspack like
module.exports = {
pnp: true | false
}
``There was a problem hiding this comment.
actually this can be passed from js side by require('module').findPnpApi and enhanced-resolve call it for every resolve call(which is time consuming, https://github.com/webpack/enhanced-resolve/blob/247edebc909040ff99200108f7165e57f4fac2b1/lib/ResolverFactory.js#L138),but with better support for monorepo(like multi .pnp.cjs support, @arcanis not sure whether multi .pnp.cjs is legit usage of yarn)
I think we can support bultin resolve plugin so uers don't use pnp don't pay the cost of pnpapi call
const rspack = require('@rspack/core');
module.exports = {
// enable pnp functionality by builtin plugin
plugins: [new rspack.pnpRspackPlugin()]
}
There was a problem hiding this comment.
Note that we could make the implementation smarter, and only check directories that haven't been checked before, assuming we keep a cache of some sort (a HashMap<Path, Option<Path>> would be enough); this would trim most of the recursion.
There was a problem hiding this comment.
do you think the cache logic should be implemented in find_closest_pnp_manifest_path or outside
packages/rspack-test-tools/tests/configCases/resolve/pnp/.yarn/install-state.gz
Outdated
Show resolved
Hide resolved
| @@ -136,7 +136,7 @@ export class JsResolver { | |||
|
|
|||
| export class JsResolverFactory { | |||
| constructor() | |||
| get(type: string, options?: RawResolveOptionsWithDependencyType): JsResolver | |||
| get(type: string, options?: RawResolveOptionsWithDependencyType,pnpManifestPath: string|undefined): JsResolver | |||
There was a problem hiding this comment.
Do you plan to move the pnpManifestPath into the option bag (here and in the other places where the manifest path is passed as free function)? Ideally the PnP-specific references should be kept to a minimum imo.
There was a problem hiding this comment.
no, don't want to change resolveOption api signature, I'm not sure whether it's worthy to support manifest per request or manifest per project. if we need to support manifest per project, we need to pass it to resolveOptions at least in rust side.
| let pnp_manifest = pnp_manifest_path | ||
| .and_then(|x| load_pnp_manifest(x).ok()) | ||
| .map(Arc::new); |
There was a problem hiding this comment.
nit: Perhaps this function should return a Result, or panic if the PnP file exists but can't be loaded (since presumably it'd cause all resolutions to fail with cryptic "module not found" errors)?
There was a problem hiding this comment.
thanks for tip, yes, it need some api change
| ) -> napi::Result<JsResolver> { | ||
| let pnp_manifest_path = find_closest_pnp_manifest_path(context); |
There was a problem hiding this comment.
Note that we could make the implementation smarter, and only check directories that haven't been checked before, assuming we keep a cache of some sort (a HashMap<Path, Option<Path>> would be enough); this would trim most of the recursion.
|
This pull request has been automatically marked as stale because it has not had recent activity. If this pull request is still relevant, please leave any comment (for example, "bump"). |
|
Bump, I still have it in my backlog (I've been in on-and-off paternity leave since August, so my bandwidth has been limited - but I'm still very interested to bring it to the finish line). |
thank you, we'll continue investigating yarn pnp support |
Summary
Don't merge this before Rspack 1.0.0
support yarn pnp feature, closes #2236
Checklist