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.