Consider removing peerDependency node-sass and enforcing it at runtime instead

[gaearon]

Hey 👋

Create React App users have been very vocal about wanting to have Sass supported out of the box.

My personal concerns about node-sass haven't changed: it’s unfortunate to impose downloading a binary on each our user, and especially unfair to subject them to build issues like sass/node-sass#2146 if they’re not even planning to rely on Sass.

But since the demand for Sass isn’t going away, I’d like to make it opt-in for our users. The ideal user experience would be that they don’t need to download node-sass by default and don’t see any peer dependency warnings, but if they attempt to import a Sass file, they will see an error message telling them to install a compatible version of node-sass.

The biggest roadblock to this experience is that sass-loader will just crash without node-sass installed. We could prevent this by putting our own custom loader in front of it, but this still doesn’t solve the scary peerDependency warning coming from our package if it depends on sass-loader but not node-sass. And, as I mentioned above, we don’t want to force our users to install node-sass unless they actually plan to use it.

Here is how sass-loader works today:

• It has a peerDependency on node-sass
• If node-sass doesn't exist, sass-loader crashes

Here is my proposal for how it could work:

• It has no peerDependency on node-sass
• The import of node-sass is wrapped in a try / catch
• If node-sass is missing or require('node-sass').info doesn’t match the compatible range, the loader issues a compilation error telling the user to install a compatible version of node-sass

This allows tools like Create React App to depend on sass-loader by default without creating hurdles for users who don’t want it. What do you think? We’d be happy to send a PR ourselves if you can get behind this change in behavior.

[jenessawhite]

Yes!

[billyjanitsch]

Why not put the equivalent "try-catch" wrapping in CRA? That is, do not have CRA depend on node-sass or sass-loader, but write a simple stub loader that is configured to run on sass files. That loader attempts to load sass-loader in a try-catch. If it succeeds, it delegates the rest of the loading to sass-loader. If it fails, print an error telling the user to install sass-loader and node-sass to enable sass support.

[gaearon]

We don’t want to expose Webpack-specific concepts (such as “loaders”) to our user until they eject, or to give an impression that it’s supported to install loaders. So any solution that requires the user to install a webpack loader into their project doesn’t work for us. node-sass is different because it is not webpack-specific, and its versioning cycle isn’t directly tied to webpack.

This would also prevent us from easily updating webpack under the hood (since that often involves updating loaders too). And we actually do that, e.g. we updated from webpack 2 to webpack 3 in a patch version (from our users’ point of view).

[taion]

That seems like a significant downgrade in experience for other users of sass-loader. If I'm installing sass-loader because I want to use it, why should I wait until runtime to see that I have an invalid dependency setup?

The complexity here seems like it would be better handled by CRA. For example, CRA's tooling could detect if there are Sass files present, then prompt the user to install some additional package that wraps the behavior up – perhaps a react-scripts-plugin-sass or something equivalent.

[billyjanitsch]

I see the concern, and I understand why you want to avoid displaying a peer dep warning by default, especially in an explicitly newbie-friendly tool like CRA. Also, I realize that there are longstanding problems with the node sass binary that are unfortunate to impose even on users who don't plan on using sass.

However, I think it's worth considering that sass-loader's peer dependency on node-sass is intentional and correct. Wrapping the load in a try-catch block that includes a semver check is re-implementing an npm feature that already exists. Similarly, the "correct" thing for CRA to do here if it wants to abstract loaders, etc. from the consumer is to depend on both sass-loader and node-sass even if they remain unused by some consumers. So I'm interested in seeing whether there's an alternative that doesn't require changes to sass-loader but still results in a good experience for CRA users.

How about something like a create-react-app-plugin-sass package, which depends on sass-loader and node-sass. Its existence could be checked in a stub sass loader in CRA like the one I described previously. (Perhaps you want to avoid introducing the idea of "plugins" to CRA -- maybe the package name could be adjusted to imply that this is not a standard pattern?)

Edit: @taion beat me to the same general idea. :)

[gaearon]

That seems like a significant downgrade in experience for other users of sass-loader. If I'm installing sass-loader because I want to use it, why should I wait until runtime to see that I have an invalid dependency setup?

Have you ever installed webpack, typed the config, and got it working right on the first try? I empathize with the concern here but I’m not sure how much practical difference it makes. You need to run it at least once to figure out what’s broken anyway.

In my opinion, printing a friendly error message that tells you exactly what to do might be a better user experience than relying on npm to show a peer dependency warning in the sea of other output like node-sass own binary installation logs.

The complexity here seems like it would be better handled by CRA. For example, CRA's tooling could detect if there are Sass files present, then prompt the user to install some additional package that wraps the behavior up – perhaps a react-scripts-plugin-sass or something equivalent.

We don’t really plan to create a plugin system. I understand react-scripts-plugin-sass could be a decoy package but that just reinforces the incorrect idea that we plan to support plugins.

How about something like a create-react-app-plugin-sass package, which depends on sass-loader and node-sass. [...] (Perhaps you want to avoid introducing the idea of "plugins" to CRA -- maybe the package name could be adjusted to imply that this is not a standard pattern?)

Another problem with this is it doesn’t let our users pin their compiler version directly (which is somewhat important given that Sass is a binary and has all sorts of weird issues like segfaults).

Similarly, the correct thing for CRA to do here if it wants to abstract loaders, etc. from the consumer is to depend on both sass-loader and node-sass even if they remain unused by some consumers. So I'm interested in seeing whether there's an alternative that doesn't require changes to sass-loader.

I understand it is technically “correct”. But forcing people to download binaries that create weird issues for a use case only some people want is objectively worse experience.

As far as I know, there is a precedent for this: I’ve heard that Parcel doesn’t install Sass by default, but prompts to install it on demand when it is used. I think we’re trying to do something similar, but indeed I don’t know if we can pull it off on top of webpack. This issue is my attempt to better understand this. I agree that maybe webpack might not be the best platform for building low-configuration solutions on top of, but it’s very popular in React ecosystem, and we wouldn’t want to migrate off of it in the near future unless absolutely necessary.

[taion]

Yeah! As a user, an ideal solution sounds something like:

• On running the start or build script, react-scripts checks for the presense of Sass files
• If Sass files are present, but react-scripts-sass is not installed, prompt the user with something like Sass files detected. Install Sass support? [Y/n]
  • If user selects y, install react-scripts-sass and restart the script
• In the webpack configs, pull in some exported Sass loader atom from the react-scripts-sass above, if present

So as a user of sass-loader, I get the dependency errors I want eagerly. Meanwhile users of react-scripts don't have to pull in sass-loader at all unless they need it, and get walked through in a nice way with a clean facade that abstracts away webpack details.

[taion]

But right now you don't get a "sea of other output". When I run yarn add sass-loader, I get:

That's pretty to-the-point.

[ahmadawais]

I came here to share exact same thoughts as shared by @taion above.

• Detect .scss/.sass files (I think .sass usage is quite less)
• Prompt user to install sass dependencies or remove sass files.
• Install Sass support? [Y/n] which leads to installation of a sass plugin or functionality via react-dev-utils.

[gaearon]

@taion For some reason I was less lucky 😢

Which is kind of my point—I don’t want CRA users to ever see something like this unless they explicitly opt in.

[xzyfer]

👋 Node sass maintainer here. Happy to help where we can. Regarding the binary download issues, there's are largely unavoidable. As such we're investigating the feasibility of converting the binary to Web Assembly so a single artefact can be packaged in the npm download.

[Tetheta]

I've had so many coworkers that have had all sorts of bizarre errors with node-sass and I've had plenty of my own issues as well.

If it does get included in CRA I would want at the least better error messages, preferably with a link to common fixes for said messages.

[taion]

In my example, I'm not installing node-sass. If you are installing node-sass, then you wouldn't get that peer dependency warning anyway.

I don't think CRA needs a full-fledged plugin system to handle these sorts of optionals. What's wrong with something in https://github.com/facebookincubator/create-react-app/blob/v1.0.17/packages/react-scripts/config/webpack.config.dev.js like:

let sassRule;
try {
  sassRule = require('react-scripts-optional-sass').rule;
} catch (e) {
  sassRule = null;
}

Then only adding sassRule if it's present?

I don't think anybody's going to mistake this for a plugin system.

[billyjanitsch]

forcing people to download binaries that create weird issues for a use case only some people want is objectively worse experience.

Yes, of course. I'm looking for a solution that doesn't require this by default, and doesn't require sass-loader to do custom stuff. Best of both worlds, if we can make it work. :) If not, I think it's totally reasonable to fall back on a solution like the one you've proposed; sorry if I came across as shooting it down unconditionally.

We don’t really plan to create a plugin system. I understand react-scripts-plugin-sass could be a decoy package but that just reinforces the incorrect idea that we plan to support plugins.

So "plugin" is not the right name. But comparing this to the solution you proposed, they are both opt-in; they both require the user to install an additional package if they want sass. And isn't more obvious to a newbie what's going on if they are asked to install react-scripts-addon-sass ("ah, I am adding sass support to CRA!"), which can print explanatory logs as it goes ("we just installed sass for you -- sorry for the mess of binary build logs")?

Another problem with this is it doesn’t let our users pin their compiler version directly (which is somewhat important given that Sass is a binary and has all sorts of weird issues like segfaults).

Just brainstorming: this could be solved by having react-scripts-addon-sass depend on node-sass, but also check whether there is a version of node-sass installed higher up the tree that also satisfies its semver range. If so, it could assume that this version has been intentionally pinned by the user, and should be used instead. react-scripts-addon-sass would be free to print console info describing how it's behaving here, to make it clear to users what's going on, and how they can pin a compiler version if they need to.

[taion]

Another problem with this is it doesn’t let our users pin their compiler version directly (which is somewhat important given that Sass is a binary and has all sorts of weird issues like segfaults).

There are lots of things that users can't do with CRA, though! "Zero-config except you pick your version of node-sass" is sort of odd, anyway.

If it came down to it, aside from ejecting, there are always Yarn resolutions. (Or installing node-sass first and hoping that the package manager does the right thing with deduping.)

[satya164]

I agree with the points @taion put here. I have nothing to add, but I'd just say that this seems like responsibility of CRA. peerDependencies are the standard way to notify about additional required packages at install time and it seems odd to replace it with an inconsistent behavior that requires users to actually try to trigger a build to know about missing dependencies just to satisfy the needs of CRA.

[gaearon]

I see, thanks for the discussion. I feel strongly that we don't want to expose our own separate addon-like package to the user, but I understand you feel strongly about this matter too.

As a compromise, we might try writing a script that forks this package and does the necessary changes so we don't have to actively maintain a fork. If we find a way to make it low-effort and automatically "turn" into the official sass-loader after ejecting, seems like it could satisfy everyone's constraints.

[jhnns]

Thanks for bringing it up and for the discussion. Just wanted to note that until now, no sass-loader maintainer/contributor has answered on this issue so I'll just add my two cents here 😁:

I totally understand CRA's point of view and I agree that a peer dependency warning is not really user friendly here. But I also think that from the sass-loader's perspective, a peer dependency is the right thing to do.

In general, I came to the conclusion that for a lot of starter projects that don't require a lot of custom configuration, a generic "auto-loader" would really be useful. It would apply a sane default configuration for the common file extensions and delegate loader requests to the respective loader. All peer dependencies, on the other hand, would be delegated to the top-level application. It could also check if the required peer dependency is present in the project's scope and throw a helpful error.

However, the auto-loader might cause unwanted peer dependency warnings. I don't know if NPM complains about missing peer dependencies if the dependency is available under node_modules but not listed as dependency in the requiring module.

[alexander-akait]

@jhnns @gaearon

Here is my proposal for how it could work:

It has no peerDependency on node-sass
The import of node-sass is wrapped in a try / catch
If node-sass is missing or require('node-sass').info doesn’t match the compatible range, the loader issues a compilation error telling the user to install a compatible version of node-sass

I think we should use this way. Why?

• It is safe.
• It is allow avoid npm message error.
• PR welcome to support dart-sass (seems he is support importer).

Also we can improve error message (Please install 'node-sass'...) if node-sass (or dart-sass) not found to avoid UI problems.

[gaearon]

The above comment makes sense to me. If multiple Sass implementations emerge (which, as I understand, is slowly happening), it would probably make sense for sass-loader to relax the peer so either of them can be used depending on an option. That is, of course, if their API is otherwise the same, and most plugin logic is independent of the Sass implementation. I don’t know whether this is the case.

To give you some perspective on why I opened this issue at all: I didn’t expect that the sass-loader community considers this peer dependency warning so useful. In my experience peer dependencies have been a pain, including inconsistent handling between package manager versions, inability to try an arbitrary beta version of the peer, and that people ignore them or don’t know how to interpret them, and still bump into inconsistency issues that need runtime solutions.

From this thread though it seems like it has been valuable to you so I won’t try to persuade you from dropping them anymore. 🙂 Or maybe this is more of a ideological debate about how this feature “should” be used in which case I fully agree—in theory my proposal doesn’t make as much sense.

[alexander-akait]

@gaearon yep, it is also allow to using alpha, beta and rc versions. PR welcome. I think @jhnns agree with me.

[alexander-akait]

Label as major because some people can't have node-sass as deps and it is break their build (example other package can install node-sass). But if somebody have other opinion - discussion welcome.

[taion]

The most analogous case I can think of there is with WebSocket-based libraries that can use either ws or uws. In the cases I can think of, the libraries depend on ws and use ws by default, but can be optionally configured to use uws.

I don't think I've ever seen a library that intentionally does not work out-of-the-box even with its peer dependencies installed, assuming those peer dependencies all live on npm.

[gaearon]

@evilebottnawi I'm not sure I see why this would be a breaking change except for npm@2 (which I'm not sure if you support). In other npm versions and Yarn, peerDependencies is only respected for warnings and don't automatically get installed. So removing a peer dependency should be possible to do in a minor release.

[alexander-akait]

@gaearon minimum supported node 4.3 (look in package.json), default version npm is 2.14.12 for 4.3 (look https://nodejs.org/en/download/releases/). I agree that very few people use such versions, but if somebody use this versions we break build(s) and it is breaking changes.

[gaearon]

This is fair, thanks for explaining.

[leonaves]

Not sure if you had considered this but an alternative approach could be to propose an NPM feature that allows you to suppress specific child deps' peer dependency warnings? Of course assuming it was added, you would still only be able to support NPM versions after that was released, which isn't great 😬 but 'twas just a thought that might prompt further discussion?

[jhnns]

@gaearon Just wanted to let you know that I'm currently re-thinking this. A hand-crafted, helpful error message is probably better than a peer dependency 👍. There is a PR to implement this #533

[franciscop-invast]

@leonaves I've tried several npm feature recommendations, all of them got closed with no official response. They seem quite overloaded and Facebook even created Yarn as an alternative to npm, I am guessing partly for how dysfunctional npm has become in the last years.

[jhnns]

#533 was shipped with sass-loader 7.0.0. There is no peer dependency on node-sass anymore.

[felixfbecker]

Coming to this thread from reading the 7.0.0 changelog and reading through all of it, I don't understand why the peer dependency was removed. Isn't warning about incompatible/missing versions at install time the whole point of peerDependencies? I dislike the thought that now if I have an incompatible sass version, I won't find out until rerunning webpack, and automated dependency updates have absolutely no way to determine this statically to coordinate an update.

[alexander-akait]

@felixfbecker in favor support dart-sass, try to implement this in near future