Yarn PnP is not compatible with optional peer dependencies

[Timer]

This makes PnP incompatible with how we're shipping Sass and planning to ship future enhancements (e.g. TypeScript).

Optional peer dependencies are vital to keeping initial install sizes low (less of a problem with PnP, but imagine a cache miss).

See broken test:

./src/AppSass.scss
- To import Sass files, you first need to install node-sass.
- Run `npm install node-sass` or `yarn add node-sass` inside your workspace.
+ Error: Package "[email protected]" (via "/Users/joe/Library/Caches/Yarn/v3/npm-sass-loader-7.1.0-16fd5138cb8b424bf8a759528a1972d72aad069d/node_modules/sass-loader/lib/loader.js") is trying to require the package "node-sass" (via "node-sass") without it being listed in its dependencies (webpack, clone-deep, loader-utils, lodash.tail, neo-async, pify, semver, sass-loader)

[Timer]

/cc @arcanis

x-ref: yarnpkg/yarn#6487

[arcanis]

I think there's two different problems, and I'm not sure they're PnP per se.

• The log your test is expecting is derived from the webpack output (cf here). Since the PnP output has more information, the code linked doesn't understand it, and that's why you get the diff. Extending the regex to cover the PnP error message should do the trick.

• That being said, as described in the error message itself, there's a second problem in that sass-loader is not listing node-sass in its dependencies (not even in its peer dependencies). If it was listed but missing, you'd get a different error message that would explain the situation.

Ironically, node-sass was a peer dependency until it got removed to make way for ... create-react-app (webpack-contrib/sass-loader#532, cc @gaearon) 😛 Adding it back should do the trick.

If I read the thread correctly, the main reason for removing it was to make the peer dependency warning go away, which is something that should be fixed at the package manager level, like what we're doing in yarnpkg/yarn#6487.

[Timer]

The test output isn't specifically what I was worried about, it more-so just brought it to light.

But yes, this is more about supporting optional peer-dependencies (yarnpkg/yarn#6487).
Adding it back as a hard peerDependency isn't an option with how things work currently. For now we'll just tell people they can't use Sass and PnP. 😄

[bugzpodder]

sass-loader also supports dart-sass as a replacement implementation of node-sass (you'll need to pass it in as a option per https://github.com/webpack-contrib/sass-loader#examples ), so the user gets to decide which one to use.

[arcanis]

@Timer 😢 Not an option because of the warning, right? Is there something else, or would fixing this in Yarn be good enough for you?

@bugzpodder It also depends on how you'll want the cra users to specify that they want to use dart-sass, but overall it shouldn't be an issue with PnP, since it's the configuration file that will make the require call, not node-sass itself (otherwise it would have to declare it as an optional peer dependency as well - once available).

[bugzpodder]

So it sounds like the initial problem is fixable by just making use of the implementation option, and pass in require('node-sass'). I can give it a test later.

[Timer]

Not an option because of the warning, right? Is there something else, or would fixing this in Yarn be good enough for you?

Yeah, the warnings are a constant source of issues for us to deal with. Especially when people start trying to resolve them themselves and start installing random dependencies left and right. We see it quite frequently.

Just Yarn support probably won't be good enough. We'll want a story for npm too.

---

Also, something to keep in mind:

sass-loader will be requiring node-sass. sass-loader is a dependency of react-scripts.
The install of node-sass will happen along side, not within, react-scripts.
Will jumping for a peer dependency two levels be an issue? Will we need to make node-sass an optional peer dependency of react-scripts to make this connection?

[arcanis]

Will we need to make node-sass an optional peer dependency of react-scripts to make this connection?

tldr Yes. Otherwise it could cause issues.

In more details, let's say we have the following situation:

• A package A with deps B, C@1
• A package D with deps B, C@2
• B depends on E
• E has a peer dependency on C

In this situation, we need to make E resolve to both C@1 and C@2, depending on whether it's called through the A branch or the D branch, so we have to virtualize it (create two "virtual" packages that points to the same location on the disk but will be instantiated twice and will be linked to different packages). To do this, we create new packages "references" (kinda like versions) unique to their specific parents. Meaning that the package E becomes B/E.

Problem is, doing this doesn't solve our issue - we still have a B/E on both the A and D branches! So the only way we can resolve the conflict once and for all is to first virtualize B (which will give us a different virtual package for each branch), and then virtualize E using the virtual packages we just created. Meaning that B will be split into A/B and D/E, and E will be further split into (A/B)/E and (D/B)/E.

Since virtualizing a package means that it will be instantiated as many times as it has a unique parent in the dependency tree, and since the PnP standard guarantees that this only happen on packages with peer dependencies (so that the behavior is 100% predictable), B is required to explicitly list a peer dependency.

[Timer]

OK -- awesome. We definitely need to figure out the npm side of this story, then.

[iansu]

Can you try the latest release and see if this has improved? If not please open a new issue.