feat: Add pnp_manifest option to resolver

[smeng9]

Summary

Fixes #10919

Related links

Merge rstackjs/rspack-resolver#141 first

Checklist

• Tests updated (or not required).
• Documentation updated (or not required).

[netlify]

✅ Deploy Preview for rspack canceled.

Built without sensitive environment variables

Name	Link
🔨 Latest commit	0767244
🔍 Latest deploy log	https://app.netlify.com/projects/rspack/deploys/695de336cb6bba00089a51df

[stormslowly]

@smeng9 We should add pnpManifest option to Rspack's resolve options to config resolver sticking to that manifest.
Beside, the document for resolve needs to be updated too.

[smeng9]

@stormslowly Since rspack-resolver does not have a formal document, I have updated the document of README.md in rstackjs/rspack-resolver@f9e2fcf

[stormslowly]

@smeng9 rspack-resolver with pnpManifest option is published as 0.6.7, you can specifier that in the cargo.toml.

And we also need to expose this option in rspack's resolve options.
Could you plz help to add the options in rspack/core and update corresponding docs.

[stormslowly]

use options.pnp_manifest.map(PathBuf::from)

[smeng9]

I have updated to use options.pnp_manifest.map(PathBuf::from)

However I think providing the default value by calling pnp::find_closest_pnp_manifest_path if no pnp_manifest is provided in the options is still necessary. This ensures pnp global cache works out of the box without any additional configuration needed

[stormslowly]

this line can be removed

[smeng9]

Since we need pnp::find_closest_pnp_manifest_path function, we need the pnp dependency

[stormslowly]

As we discussed in the link provided (rstackjs/rspack-resolver#141 (comment)), we need to consider the implications of sticking to the current project PnP manifest. Doing so could potentially disrupt project references to other PnP-enabled projects.

In my opinion, if a user wants to utilize the global cache, they should explicitly configure their settings—in other words, stick to the manifest file.

However, if we config the manifest for uesrs, they may encounter difficulties when working with projects that reference other Yarn projects.

[stormslowly]

I am thinking that Yarn PnP's enableGlobalCache is enabled by default. https://yarnpkg.com/configuration/yarnrc#enableGlobalCache

So the PnP manifest should support global cache by default.

1. pnpManifest: null/undefined, rspack fill it current path's PnP manifest file path
2. pnpManfiest: some string path, use the config manfiest
3. pnpManfiest: false , support cross referenced enableGlobalCache disabled projects.

But case 1, will be a breaking change for current PnP users.
I think We can introduce supporting GlobalCache enabled Pnp project by default in Rspack 2.0.

So what's your opinion @smeng9

cc @LingyuCoder @chenjiahan

[smeng9]

Hi @stormslowly

This merge request is a bug fix for incorrectly resolved .pnp.cjs in global cache, it will not introduce breaking change.
This is based on the assumption that there is no .pnp.cjs file in the global cache folder, and the user will only use the closest .pnp.cjs file from current path's PnP manifest file path

In rspack-resolve https://github.com/rstackjs/rspack-resolver/blob/main/src/lib.rs#L899C19-L899C49, we are already resolving .pnp.cjs from current path's PnP manifest file path. But if the file is in global folder, it does not have .pnp.cjs. We just lifted this resolving up from rspack-resolve to rspack, so the current folder is based on the project rather than the global cache.

Even if we have a yarn PnP project listed as another yarn PnP project's dependency, when performing a yarn install, the dependent project will be installed to global cache first, which strips away its .pnp.cjs

[stormslowly]

Let me clarify my explanation with the following example: GitHub Repository.

.
├── app
│   ├── dist
│   │   ├── main.js
│   │   └── main.js.map
│   ├── index.js
│   ├── package.json
│   ├── README.md
│   ├── rspack.config.js
│   ├── .pnp.cjs
│   └── yarn.lock
├── lib
│   ├── index.js
│   ├── package.json
│   ├── README.md
│   ├── .pnp.cjs
│   └── yarn.lock
└── README.md

Both the app and lib directories enable Plug'n'Play (PnP) without a global cache. The app/index.js file requires lib/index.js, and lib/index.js requires the npm package is-sorted.

The app project works with the configuration resolve: { pnp: true }, because, in PnP mode, the resolver finds the PnP manifest from the current path. Specifically:

• It uses app/.pnp.cjs when resolving lodash from app/index.js.
• It uses lib/.pnp.cjs when resolving is-sorted from lib/index.js.

However, it fails when using the current Pull Request due to the configuration of the PnP manifest in the rspack-resolver, which causes the resolver to only reference the app/.pnp.cjs file. In other words, the resolution for is-sorted from lib/index.js still relies on app/.pnp.cjs, leading to the failure.

I hope this explanation is clearer.

$ yarn node /path/to/rspack/packages/rspack-cli/bin/rspack.js

ERROR in ../lib/index.js 1:26-36
  × Module not found: Can't resolve 'is-sorted' in '/Users/bytedance/git/problem/yarn_pnp_test/lib'
   ╭─[1:17]
 1 │ const isSorted = require("is-sorted");
   ·                  ────────────────────
 2 │
 3 │ module.exports = function (str) {
   ╰────

Rspack compiled with 1 error in 66 ms

[smeng9]

I see the problem. If this is considered a breaking change, we can push it to 2.0

However I still think if app/index.js uses ../lib/index.js directly by referencing a single file out of it's package scope, which has hidden imports of is-sorted, then is-sorted should be declared in app's package.json's dependency, otherwise the require call is ambiguous and unsound.

[smeng9]

If we are setting up the package to include files from another package, we should do the following stormslowly/mult_yarn_pnp_manifest@master...smeng9:mult_yarn_pnp_manifest:master