node-sass peer dependency

[arcanis]

Expected Behavior / Situation

The sass-loader package requires node-sass, and as such node-sass should be a dependency (or a peer dependency).

Actual Behavior / Situation

The sass-loader package doesn't list node-sass at all.

Modification Proposal

The peer dependency should be listed. I've seen that removing it was a conscious choice in #532, hence why I'm opening this issue rather than a PR (I can open a PR once we agree) cc @gaearon

Note that the problem described in #532 is partially fixed already - Yarn supports optional peer dependencies through the use of the peerDependenciesMeta field. We have an intent to implement from pnpm, only remains npm which doesn't communicate much anyway.

[alexander-akait]

Already discussed, no, we can't add node-sass to peer deps, because you can use sass (dart-sass) package. Adding node-sass to peer deps break app like create-react-app, next.js and etc

Oh, my mistake, let' discussion about pnp support.

[arcanis]

As mentioned the situation is different because optional peer dependencies are a thing now. You could potentially list both node-sass and dart-sass as peer dependency (the package manager wouldn't warn if they are missing, only if it exists and the range doesn't match).

[alexander-akait]

@arcanis it is bad solution, we will get many issue about this warning

[arcanis]

It would look like this:

{
  "peerDependencies": {
    "node-sass": "*"
  },
  "peerDependenciesMeta": {
    "node-sass": {
      "optional": true
    }
  }
}

[alexander-akait]

@arcanis can you give link on peerDependenciesMeta documentation?

[alexander-akait]

We can't add something in peerDependencies

[alexander-akait]

Also we can't break npm (even if they develop slowly), we should prefer stable/lts solution rather new feature like pnp

[alexander-akait]

We can't use peerDependenciesMeta field, it is requires to use peerDependencies, we need to search solution where we don't add anything in peerDependencies

[arcanis]

@arcanis can you give link on peerDependenciesMeta documentation?

At the moment the best documentation is the merged RFC. It's fairly complete so I didn't put up a duplicate page on the website yet.

Also we can't break npm (even if they develop slowly)

They wouldn't break, they would start to get a warning. The reason I don't personally see adding something new to peerDependencies as a blocker are:

• If you see it as a breaking change, then I guess it can be postponed to the next major bump.
• The version of node-sass used by your users is supposed to be locked (even with npm), meaning that adding this new peer dependency won't cause new warnings to appear until your users consciously upgrade sass-loader. This change would mostly affect new projects.

As a side note I'm sure npm would be more enthusiast about fixing things if were held accountable for it (just like we've implemented the optional peer dependencies because we've seen that people were using hacks instead). Hiding warnings under the carpet doesn't help your users.

we should prefer stable/lts solution rather new feature like pnp

Note that PnP isn't broken. It works fine, even with node-sass and even with create-react-app (because we have a special case for it, which you'll agree would be better avoided).

My point is that by not listing the peer dependency, you're actively making use of an undefined behavior. It's a dangerous thing, even outside of the PnP mode. Peer dependencies have actual implications regarding how the package manager decides to hoist the packages in the dependency tree, and not listing them leaves the door open to errors super-hard to understand.

This issue isn't about PnP (which, again, works fine). I'm just another open-source lover that cares about seeing the ecosystem improve incrementally and in a scalable way instead of building layers of hacks 🙂

[alexander-akait]

/cc @arcanis so sass-loader works as expected with pnp (if you add node-sass/sass in your dev deps), right?

[yangxuanxing]

In our project, we flatten all dependencies to the top node_modules folder.
as node-sass is not in the dependencies, when excute, it cannot resolve the module