refactor(package): remove `node-sass` (`peerDependencies`) #533

Timer · 2018-01-15T20:18:19Z

Fixes #532

Let me know what more is required. Doc changes, fixing coverage, etc.

jsf-clabot · 2018-01-15T20:18:19Z

All committers have signed the CLA.

codecov · 2018-01-15T20:25:26Z

Codecov Report

Merging #533 into master will increase coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #533      +/-   ##
==========================================
+ Coverage   97.43%   97.58%   +0.14%     
==========================================
  Files           6        6              
  Lines         117      124       +7     
==========================================
+ Hits          114      121       +7     
  Misses          3        3

Impacted Files	Coverage Δ
lib/loader.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb5a555...2cb77d9. Read the comment docs.

alexander-akait · 2018-01-16T09:35:56Z

lib/loader.js

+    const sass = require("node-sass");
+    const sassVersion = require("node-sass/package.json").version;
+
+    if (/^4[.]/.test(sassVersion)) {


I think better to check >= 4, maybe in future we can have 5

Yep, please check for greater than

if (Number(sassVersion[0]) >= 4) {}

What if we're not compatible with the node-sass v5 API?

michael-ciniawsky · 2018-01-16T09:52:17Z

lib/loader.js

+    const sass = require("node-sass");
+    const sassVersion = require("node-sass/package.json").version;
+
+    if (/^4[.]/.test(sassVersion)) {


Yep, please check for greater than

if (Number(sassVersion[0]) >= 4) {}

michael-ciniawsky · 2018-01-16T09:53:20Z

lib/loader.js

+    if (/^4[.]/.test(sassVersion)) {
+        asyncSassJobQueue = async.queue(sass.render, threadPoolSize - 1);
+    } else {
+        loadingError = new Error("The installed version of \`node-sass\` is not compatible (expected: 4.x, actual: " + sassVersion + ").");


throw here and catch the {Error} in (L27+) instead of an assignment please

Good point. This will work better when we add support for things other than node-sass or something else bombs out.

Timer · 2018-01-16T13:21:36Z

Updated with concerns.

alexander-akait · 2018-01-16T13:36:37Z

lib/loader.js

@@ -12,7 +11,21 @@ const pify = require("pify");
 // fs tasks when running the custom importer code.
 // This can be removed as soon as node-sass implements a fix for this.
 const threadPoolSize = process.env.UV_THREADPOOL_SIZE || 4;


Let move this variables before asyncSassJobQueue = async.queue(sass.render, threadPoolSize - 1);, it don't have sense in top of file

Makes sense. I should've thought of this. 😝

alexander-akait · 2018-01-16T13:49:32Z

lib/loader.js

@@ -35,6 +49,15 @@ function sassLoader(content) {
        throw new Error("Synchronous compilation is not supported anymore. See https://github.com/webpack-contrib/sass-loader/issues/333");
    }

+    if (asyncSassJobQueue == null) {


Last note 😄 Seems we can avoid this block, just throw error in catch instead loadingError = e;

Is it safe to throw an error at require-time? I figured you'd want them surfaced as webpack compiler errors -- I'm not sure how this works.

@Timer Yep, it is not compilation error. It does not make sense because all attempts to compile sass/scss return error in callback. I think better throw error as fast as possible. @michael-ciniawsky what do you think about this?

Wouldn't webpack's bail option cause it to stop on the first? I'm no expert here, just asking questions.

I also think this block is redundant with the try/catch (L13+) and version check, but not 💯 😛

- let loadingError = null; try { const sass = require("node-sass"); const sassVersion = require("node-sass/package.json").version; if (Number(sassVersion[0]) >= 4) { // This queue makes sure node-sass leaves one thread available for executing // fs tasks when running the custom importer code. // This can be removed as soon as node-sass implements a fix for this. const threadPoolSize = process.env.UV_THREADPOOL_SIZE || 4; asyncSassJobQueue = async.queue(sass.render, threadPoolSize - 1); + // Maybe not needed (see comment) + if (asyncSassJobQueue === null) { + throw new Error("Please install `node-sass >= 4.x` to use `sass-loader`.") + } } else { throw new Error("The installed version of \`node-sass\` is not compatible (expected: >=4.x, actual: " + sassVersion + ")."); } } catch (err) { + // Catches 'both' (first thrown) {Error} thrown in the try block + callback(err); + return; } ... - if (asyncSassJobQueue == null) { - if (loadingError) { - callback(loadingError); - return; - } - callback(new Error("Please install `node-sass >=4.x` to use `sass-loader`.")); - return; - }

But what's the exact reason for asyncQueue === null, in case node-sass < 4 ? And isn't this basically a redundant (the same check) as if (Number(sassVersion[0]) >= 4) again ? Please elaborate what could possibly fail when queueing sass.render for thread pooling, since I'm not that familiar with node-sass :)

Callback isn't available where you've added it, that's where the package itself is being evaluated.

AFAIK, it's generally poor practice to make a require throw an exception, but this might be normal practice in the webpack world.

The loadingError is to store the error until the plugin is used at compile time.

Ooops... 😛you're right. What alternatives do we have to make this less confusing and more compact ?

I'll come up with something.

Timer · 2018-01-17T13:45:39Z

@michael-ciniawsky I went ahead and moved the loading of a compiler into its own file and created placeholder logic for expanding beyond one compiler. This keeps the main loader.js clean while allowing it to be extensible going forward.

Let me know what you think!

alexander-akait

Can we add tests?

Timer · 2018-01-17T14:08:35Z

Can we add tests?

Sure! I just want to make sure the code is accepted and no functionality majorly changes before I write the test cases. 😄

alexander-akait · 2018-01-17T14:45:39Z

@michael-ciniawsky friendly ping

Timer · 2018-01-19T15:40:49Z

@michael-ciniawsky could you please see if you agree with the changes? 😄

jhnns

@Timer Thanks for your contribution. I'm willing to merge this since it seems like this could improve the overall flexibility and developer experience. It also helps the maintainers of create-react-app to support Sass out of the box (#532) @gaearon.

This is going into the right direction, but I'm not sure how big the change should be. Currently, this is built with the flexibility in mind to support multiple compilers. However, this is only going to work if all these node packages provide roughly the same API/behavior as node-sass. Could you name some node packages for example so that I can get a better understanding of that requirement?

If a sass compiler/node package exposes a completely different API, it probably makes more sense to have a dedicated loader for it.

So, if there are no other packages that provide the same API, we should probably make the change smaller and just check specifically for node-sass.

Besides, we also have to keep in mind that the current sass-loader implementation applies some fixes that expect a specific node-sass behavior.

What do you think?

jhnns · 2018-02-17T14:36:27Z

.nycrc

@@ -6,9 +6,9 @@
    "include": [
        "lib/**/*.js"
    ],
-    "lines": 97,
+    "lines": 92,


I don't think we should just decrease the coverage limit here 😁
The new code should have tests :). Maybe we should add each supported compiler as dev dependency. Then requiring it in a test should be no problem.

jhnns · 2018-02-17T14:39:00Z

lib/compilers/index.js

+        try {
+            const value = compiler.getCompilerQueue();
+
+            if (value == null) {


I'm not a fan of these == null checks (the V8 for instance has a hard time optimizing these since document.all == null evaluates to true 😁). Why not check on === null instead? We know that the compiler queue should return null, so let's enforce a strict API.

Fair enough.

jhnns · 2018-02-17T14:42:51Z

lib/compilers/index.js

+    }
+    // Return the error encountered while loading a compiler, otherwise say
+    // there was no compiler found.
+    return {


Why not just return the queue here or throw or log the error if none was available?

AFAIK throwing on require is being a bad neighbor, so we store the error until the loader is invoked (this function is called synchronously on package evaluation).

jhnns · 2018-02-17T14:43:37Z

lib/compilers/libsass.js

@@ -0,0 +1,27 @@
+"use strict";


Could you rename that file to node-sass. The sass-loader doesn't really know about libsass

jhnns · 2018-02-17T15:06:49Z

lib/loader.js

@@ -35,6 +30,13 @@ function sassLoader(content) {
        throw new Error("Synchronous compilation is not supported anymore. See https://github.com/webpack-contrib/sass-loader/issues/333");
    }

+    const compilerError = asyncJobQueue.error;
+
+    if (compilerError) {


No need to do that check. If the error was just thrown, webpack would report the loader error accordingly.

alexander-akait · 2018-03-24T10:56:34Z

/cc @Timer let's finish this PR and implement dart-sass support

Timer · 2018-03-25T20:45:12Z

I believe dart-sass support would be better in a PR after this. I toyed with the idea (see tree at d2dae27) but I think it'll require more significant changes (such as the error message massaging and fixing library-specific quirks).

Let's keep this PR focused to removing the peer dep.

Timer · 2018-03-25T21:16:24Z

@jhnns I've greatly simplified this PR and added tests; can I ask for another review?

Timer · 2018-03-25T22:09:21Z

Remaining tests should pass after #514 is merged.

alexander-akait · 2018-03-26T09:18:49Z

/cc @michael-ciniawsky

Timer · 2018-04-10T19:52:37Z

Hi! Anything we can do to help this along?

We'd really like to add Sass support in Create React App, but we need this released first.

alexander-akait

Only one note 👍

alexander-akait · 2018-04-10T20:06:47Z

.nycrc

    "functions": 100,
-    "branches": 89,
+    "branches": 91,


Can we return old values and add more tests?

Just to clarify, you want me to reduce target test coverage and add then add more tests?

I increased the values here, I did not decrease them -- the tests I added provided more coverage than previously available.

@Timer oh sorry, all looks good 👍

jhnns · 2018-04-12T20:54:29Z

LGTM 👍 thank you 👏

Travis is failing right now since it's trying to run webpack@4 with node 4 which doesn't work... I'll try to get #514 merged so that our CI is working again.

- Add mock-require

jhnns · 2018-04-13T00:29:28Z

I will release this tomorrow together with #479 #555

geoffreydhuyvetters · 2018-04-13T06:49:06Z

this is a massive PR, lots of people will be happy :)

jhnns · 2018-04-13T07:18:43Z

Shipped with sass-loader 7.0.0. There is no peer dependency on node-sass anymore.

track0x1 · 2018-09-19T21:04:03Z

package.json

@@ -54,7 +55,6 @@
    "node": ">= 6.9.0 || >= 8.9.0"
  },
  "peerDependencies": {
-    "node-sass": "^4.0.0",


Why did you remove node-sass as a peerDependency when sass-loader explicitly requires a version?

sass-loader requires either node-sass or sass. There's no way to express that as a peer dependency without having npm complain about one or the other missing.

Ah! Thank you. Didn't know you could use either.

alexander-akait reviewed Jan 16, 2018

View reviewed changes

alexander-akait requested review from jhnns, michael-ciniawsky and joshwiens January 16, 2018 09:36

alexander-akait added type: Feature pr_action: Review priority: 5 (nice to have) semver: Major labels Jan 16, 2018

michael-ciniawsky requested changes Jan 16, 2018

View reviewed changes

michael-ciniawsky changed the title ~~Remove node-sass peerdep~~ refactor(package): remove node-sass (peerDependencies) Jan 16, 2018

michael-ciniawsky added type: Refactor and removed type: Feature labels Jan 16, 2018

Timer force-pushed the remove-peerdep branch from 6c803e7 to 2cb3fa3 Compare January 16, 2018 13:30

alexander-akait reviewed Jan 16, 2018

View reviewed changes

gaearon mentioned this pull request Jan 16, 2018

Optional Sass support - Part 2 facebook/create-react-app#2498

Closed

alexander-akait reviewed Jan 17, 2018

View reviewed changes

jhnns requested changes Feb 17, 2018

View reviewed changes

jhnns mentioned this pull request Feb 17, 2018

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

Closed

Fabianopb mentioned this pull request Mar 21, 2018

Sass loader facebook/create-react-app#4195

Merged

alexander-akait mentioned this pull request Mar 24, 2018

dart-sass? #435

Closed

Move node-sass resolve to runtime

a810cbd

Timer added 4 commits March 25, 2018 16:46

Rip out changes

9efb591

Remove rest of changes

33ee227

Defer setting of asyncSassJobQueue

82acdca

Oops

96d0730

Timer added 3 commits March 25, 2018 17:59

Add tests

1a6b8cc

Bump min coverage requirements

2df1529

Make tests pass with webpack 4

dd0d28b

BrodaNoel mentioned this pull request Mar 27, 2018

[WIP] refactor(loader): v7.0.0 #514

Closed

alexander-akait approved these changes Apr 10, 2018

View reviewed changes

alexander-akait requested changes Apr 10, 2018

View reviewed changes

alexander-akait approved these changes Apr 12, 2018

View reviewed changes

jhnns approved these changes Apr 12, 2018

View reviewed changes

jhnns mentioned this pull request Apr 12, 2018

Fix CI #555

Merged

jhnns added 2 commits April 12, 2018 23:45

Merge branch 'master' into pr/origin/533

288b272

chore: Update package-lock.json

2cb77d9

- Add mock-require

jhnns merged commit 6439cef into webpack-contrib:master Apr 12, 2018

Timer deleted the remove-peerdep branch April 13, 2018 11:31

This was referenced Jul 12, 2018

Remove reference to node-sass as a peerDependency #589

Closed

node-sass should be a dependency or a peerDependency, not a devDependency? #590

Closed

track0x1 reviewed Sep 19, 2018

View reviewed changes

edmorley mentioned this pull request Oct 26, 2018

Style loader for sass / scss does not work neutrinojs/neutrino#1066

Closed

refactor(package): remove node-sass (peerDependencies) #533

refactor(package): remove node-sass (peerDependencies) #533

Conversation

Timer commented Jan 15, 2018 • edited Loading

jsf-clabot commented Jan 15, 2018 • edited Loading

codecov bot commented Jan 15, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Timer commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-ciniawsky Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Timer commented Jan 17, 2018

alexander-akait left a comment

Choose a reason for hiding this comment

Timer commented Jan 17, 2018

alexander-akait commented Jan 17, 2018

Timer commented Jan 19, 2018

jhnns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Timer Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-akait commented Mar 24, 2018

Timer commented Mar 25, 2018

Timer commented Mar 25, 2018 • edited Loading

Timer commented Mar 25, 2018

alexander-akait commented Mar 26, 2018

Timer commented Apr 10, 2018

alexander-akait left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Timer Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhnns commented Apr 12, 2018

jhnns commented Apr 13, 2018

geoffreydhuyvetters commented Apr 13, 2018

jhnns commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refactor(package): remove `node-sass` (`peerDependencies`) #533

refactor(package): remove `node-sass` (`peerDependencies`) #533

Timer commented Jan 15, 2018 •

edited

Loading

jsf-clabot commented Jan 15, 2018 •

edited

Loading

codecov bot commented Jan 15, 2018 •

edited

Loading

michael-ciniawsky Jan 16, 2018 •

edited

Loading

Timer Mar 25, 2018 •

edited

Loading

Timer commented Mar 25, 2018 •

edited

Loading

Timer Apr 12, 2018 •

edited

Loading