-
-
Notifications
You must be signed in to change notification settings - Fork 431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make this package implementation-agnostic #573
Conversation
Codecov Report
@@ Coverage Diff @@
## master #573 +/- ##
==========================================
+ Coverage 97.54% 97.81% +0.26%
==========================================
Files 6 6
Lines 122 137 +15
==========================================
+ Hits 119 134 +15
Misses 3 3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for PR! Good work!
CHANGELOG.md
Outdated
}] | ||
} | ||
}; | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working docs, but please remove this, we can do this before release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
README.md
Outdated
{ | ||
loader: "sass-loader", // compiles Sass to CSS | ||
// require("node-sass") instead to use Node Sass | ||
options: {sass: require("sass")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be option. sass-loader
should resolve node-sass
, then try to resolve dart-sass
, if not found nothing - throw error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? Not allowing users to explicitly control this will mean that if they (or one of their dependencies if they're using flat dependencies) requires Node Sass for some unrelated reason, they're forced to use Node Sass. This may even result in them unexpectedly switching to Node Sass down the line.
It seems like several users would prefer making this configuration explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nex3 Hm, yes you are right it can lead to problems, let's rename sass
to implementation
and load node-sass
by default. It is allow to upgrade loader
without break build and avoid unnecessary major
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. It's worth noting though that the name implementation
, in addition to being more of a pain to type, means that users can't just write
const sass = require("sass");
// ...
options: { sass }
// ...
README.md
Outdated
loader: "sass-loader", | ||
options: { | ||
sass: require("sass"), | ||
fiber: Fiber |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to internal behavior (no new options), please provide benchmark for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean by "move this to internal behavior". Do you want sass-loader
to depend on fibers
itself? Be aware that fibers
requires native compilation, and may be difficult to install on some systems.
I'm not sure what kind of benchmarks you're looking for, but as the maintainer of Dart Sass, I can assure you that a 2x speed hit is what we measure pretty consistently when using the asynchronous code path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nex3 okey, let's leave this as is
test/index.test.js
Outdated
@@ -6,6 +6,7 @@ const path = require("path"); | |||
const webpack = require("webpack"); | |||
const fs = require("fs"); | |||
const merge = require("webpack-merge"); | |||
const sass = require("node-sass"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests should be running on node-sass
an dart-sass
together, we need change test
script:
- Install
node-sass
and run tests - Install
dart-sass
and run tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to do that gracefully, especially if the implementation isn't supplied as an option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nex3 skip tests where dart-sass
doesn't have options/features/etc and add comments about this. Why? After release all people starting upgrade and we will get a lot of issues like "It is woks on x.x.x
version, but after upgrade to x.x.x
it is doesn't works. [A lot of angry words]" (a lot of people don't read changelog and release pages). We should prepare to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but at a higher level I'm not sure how to run these tests with both versions of Sass or how to switch between those versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just add additional look here https://github.com/webpack-contrib/sass-loader/blob/master/test/index.test.js#L35 with node-sass
and sass
values and we need some modify tests to allow using difference implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure I understand. Can you give me an example of what a test you're envisioning would look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this (pseudo code):
implementations.forEach(implementation => {
syntaxStyles.forEach(ext => {
loaderOptions.implementation = implementation
// tests
});
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add implementations.forEach
here https://github.com/webpack-contrib/sass-loader/blob/master/test/index.test.js#L112
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
package.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "sass-loader", | |||
"version": "7.0.1", | |||
"version": "8.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't change this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/loader.js
Outdated
throw new Error("Dart Sass version " + version + " is incompatible with ^1.3.0."); | ||
} | ||
} else if (implementation === "node-sass") { | ||
if (major < 4 || major > 4) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide information why node-sass > 4
is not compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node Sass 5 hasn't been released yet. According to semver, it may introduce arbitrary breaking changes, so it's not safe to eagerly declare compatibility with it.
lib/loader.js
Outdated
|
||
const implementation = parsed[1]; | ||
const version = parsed[2]; | ||
const components = /^(\d+)\.(\d+)/.exec(version); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use semver
package for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes seem to have made branch coverage on loader.js
fall below the threshold, but I'm not sure why since I only changed the conditions without changing any branches.
CHANGELOG.md
Outdated
}] | ||
} | ||
}; | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
README.md
Outdated
{ | ||
loader: "sass-loader", // compiles Sass to CSS | ||
// require("node-sass") instead to use Node Sass | ||
options: {sass: require("sass")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? Not allowing users to explicitly control this will mean that if they (or one of their dependencies if they're using flat dependencies) requires Node Sass for some unrelated reason, they're forced to use Node Sass. This may even result in them unexpectedly switching to Node Sass down the line.
It seems like several users would prefer making this configuration explicit.
README.md
Outdated
loader: "sass-loader", | ||
options: { | ||
sass: require("sass"), | ||
fiber: Fiber |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean by "move this to internal behavior". Do you want sass-loader
to depend on fibers
itself? Be aware that fibers
requires native compilation, and may be difficult to install on some systems.
I'm not sure what kind of benchmarks you're looking for, but as the maintainer of Dart Sass, I can assure you that a 2x speed hit is what we measure pretty consistently when using the asynchronous code path.
lib/loader.js
Outdated
|
||
const implementation = parsed[1]; | ||
const version = parsed[2]; | ||
const components = /^(\d+)\.(\d+)/.exec(version); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/loader.js
Outdated
throw new Error("Dart Sass version " + version + " is incompatible with ^1.3.0."); | ||
} | ||
} else if (implementation === "node-sass") { | ||
if (major < 4 || major > 4) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node Sass 5 hasn't been released yet. According to semver, it may introduce arbitrary breaking changes, so it's not safe to eagerly declare compatibility with it.
package.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "sass-loader", | |||
"version": "7.0.1", | |||
"version": "8.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
test/index.test.js
Outdated
@@ -6,6 +6,7 @@ const path = require("path"); | |||
const webpack = require("webpack"); | |||
const fs = require("fs"); | |||
const merge = require("webpack-merge"); | |||
const sass = require("node-sass"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to do that gracefully, especially if the implementation isn't supplied as an option.
lib/loader.js
Outdated
@@ -62,6 +41,16 @@ function sassLoader(content) { | |||
addNormalizedDependency | |||
)); | |||
|
|||
if (asyncSassJobQueue === null) { | |||
if (!options.sass) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better rename to implementation
, sass
is very misleading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And should be node-sass
by default (comment above).
lib/loader.js
Outdated
* @param {string} info | ||
*/ | ||
function validateSassVersion(info) { | ||
const parsed = /^([^\t]+)\t([^\t]+)/.exec(info); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use regex here, please use semver
for parsing major
and minor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nex3 looks we still use /^([^\t]+)\t([^\t]+)/.exec(info)
, better use semver.major
, semver.minor
and semver.patch
functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This last regex isn't parsing the semver itself, it's parsing the implementation name and version number from the info string. I could also use info.split("\t")
if you want to avoid any regexes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nex3 maybe we can do this without regex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
README.md
Outdated
{ | ||
loader: "sass-loader", // compiles Sass to CSS | ||
// require("node-sass") instead to use Node Sass | ||
options: {implementation: require("sass")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's left default example as is and improve section about dart-sass
Like this:
<h2 align="center">Examples</h2>
### `node-sass` default
// Description and example
### `dart-sass`
// Description and example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do that if you want, but from the perspective of the Sass organization, I'd really like to present Dart Sass and Node Sass as equal options. Making Node Sass the default for backwards-compatibility makes sense, but in terms of end-user messaging I think it makes more sense to frame it as something each user should explicitly choose.
Also, from the perspective of end users, I think most people will find a lot of value in starting with Dart Sass because it's easy to install and moving to LibSass if and when they need a performance boost. Presenting the default documented workflow as LibSass first means you'll have some percentage of users whose first interaction with sass-loader
is installation woes caused by native compilation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nex3 please do this, we switch on dart sass in next major release. We should try dart sass in wild before release major.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Good work! Thanks! |
test/index.test.js
Outdated
@@ -6,6 +6,7 @@ const path = require("path"); | |||
const webpack = require("webpack"); | |||
const fs = require("fs"); | |||
const merge = require("webpack-merge"); | |||
const sass = require("node-sass"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this (pseudo code):
implementations.forEach(implementation => {
syntaxStyles.forEach(ext => {
loaderOptions.implementation = implementation
// tests
});
})
test/index.test.js
Outdated
@@ -6,6 +6,7 @@ const path = require("path"); | |||
const webpack = require("webpack"); | |||
const fs = require("fs"); | |||
const merge = require("webpack-merge"); | |||
const sass = require("node-sass"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add implementations.forEach
here https://github.com/webpack-contrib/sass-loader/blob/master/test/index.test.js#L112
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to have the coverage failing by a small margin again, and I'm not sure what to do about that.
README.md
Outdated
{ | ||
loader: "sass-loader", // compiles Sass to CSS | ||
// require("node-sass") instead to use Node Sass | ||
options: {implementation: require("sass")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
lib/loader.js
Outdated
* @param {string} info | ||
*/ | ||
function validateSassVersion(info) { | ||
const parsed = /^([^\t]+)\t([^\t]+)/.exec(info); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
test/index.test.js
Outdated
@@ -6,6 +6,7 @@ const path = require("path"); | |||
const webpack = require("webpack"); | |||
const fs = require("fs"); | |||
const merge = require("webpack-merge"); | |||
const sass = require("node-sass"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@nex3 don't look on coverage, also please rebase 👍 |
Rather than always loading Node Sass, this now requires users to pass in either Dart Sass or Node Sass as an option to the loader. Closes webpack-contrib#435
Importing stylesheets without extensions is non-standard behavior that's not supported in Dart Sass (see sass/libsass#2672).
Done! |
@evilebottnawi Is there anything left to be done here? |
@nex3 sorry for delay, review tomorrow, a lot of work, thanks for work! |
Friendly ping! |
test/sass/import-css.sass
Outdated
// 2. It does not matter whether the CSS-file exists or not, the file is just linked | ||
@import ./does/not/exist.css | ||
// 3. When the .css extension is missing, the file is included just like scss | ||
@import ~css/some-css-module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why some tests will be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CSS imports are not currently supported by Dart Sass. CSS imports with the .css
suffix are also slated for removal from LibSass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nex3 can we left css tests only for node-sass
, we remove this after upgrade node-sass
to 5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how, since all these test files are shared between the two implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nex3 need investigate, will be great if you do this, otherwise i can try this too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely don't know how to do it without a huge refactor of how tests are run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@evilebottnawi - do you have any ETA when you might be able to look at this? :) Or, is it possible to move forward without this, as the features being tested are soon to deprecate anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TroyAlford need @jhnns review
BTW Great work! Thanks! |
README.md
Outdated
@@ -24,11 +24,17 @@ Looking for the webpack 1 loader? Check out the [archive/webpack-1 branch](https | |||
<h2 align="center">Install</h2> | |||
|
|||
```bash | |||
npm install sass-loader node-sass webpack --save-dev | |||
npm install sass-loader sass webpack --save-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why there is dart sass installation if node-sass is the default implementation?
"sass-loader" // compiles Sass to CSS, using Node Sass by default
later in an example in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I missed this. Changed back to node-sass
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, the PR looks pretty good overall 🎉 👍
Besides my comment to fiber and the threadpool, I have another question: Why did you rename test/node_modules/module to test/node_modules/module.scss? The missing file extension was on purpose.
Anyway, I'm pretty confident that we can merge this soon. Thanks again for your hard work 👍
callbacks. To avoid this overhead, you can use the | ||
[`fibers`](https://www.npmjs.com/package/fibers) package to call asynchronous | ||
importers from the synchronous code path. To enable this, pass the `Fiber` class | ||
to the `fiber` option: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since webpack uses the asynchronous compilation, can we automate that process? So that we just require("fiber")
and apply the option automatically from normalizeOptions
? I guess that every dart-sass user would want to have that speedup. If require("fiber")
fails, we could instruct the user to install fiber
and point a link to the dart-sass README.
In this README, I'd just mention something like "be sure to have fibers installed in your project, see dart-sass README" (with link).
What do you think?
We'd need to add fibers as dev dependency then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fiber is a native package, so including this by default would defeat the value of this change - not having C++ code in dependencies, make SASS as easy to use as Less or Stylus.
The caveats of native packages was one of the reasons to not include SASS by default into create-react-app: facebook/create-react-app#78 (comment)
The current documentation seems fine. When users would ask "compilation of my SASS is too slow, how can I speed this up?", they will be able to find an answer. But making fibers strong recommendation or even mandatory would sound like "You should buy Ferrari when you simply need a car".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @just-boris; there's a lot of value in providing an easy pure-JS set-up experience, and then letting performance-conscious users focus on that when they need it.
Something to consider, probably after this PR lands, would be adding an option to run Dart Sass in synchronous mode, which is efficient without the use of fibers (at the cost of not supporting async importers or custom functions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fiber is a native package, so including this by default would defeat the value of this change - not having C++ code in dependencies, make SASS as easy to use as Less or Stylus.
Good catch 👍 I totally agree.
Something to consider, probably after this PR lands, would be adding an option to run Dart Sass in synchronous mode, which is efficient without the use of fibers (at the cost of not supporting async importers or custom functions).
Yes, I'm thinking about moving away from async importers. This would be a good fit.
lib/loader.js
Outdated
const implementation = options.implementation || require("node-sass"); | ||
|
||
validateSassVersion(implementation.info); | ||
asyncSassJobQueue = async.queue(implementation.render, threadPoolSize - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The async job queue mitigates a problem of node-sass/libsass where the threadpool gets exhausted (see sass/node-sass#857 (comment)). Do you know if dart-sass suffers from the same problem? (I don't think so, unless you ask for a thread from libuv like node-sass does)
If not, we could get rid of the queue for dart-sass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe Dart Sass suffers from that problem, but I wanted to keep the code path as similar between the two implementations as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable, but I suspect the queue to be one of the performance bottlenecks as discussed in #296. However, that's not a blocker for this PR.
lib/loader.js
Outdated
throw new Error("Unknown implementation \"" + implementation + "\"."); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
test/index.test.js
Outdated
}); | ||
describe("prepending data", () => { | ||
it("should extend the data-option if present", () => execTest("prepending-data", { | ||
data: "$prepended-data: hotpink" + (ext == "sass" ? "\n" : ";") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch 👍
// @see https://github.com/webpack-contrib/sass-loader/issues/368#issuecomment-278330164 | ||
{ loader: pathToSassLoader, options: {} } | ||
] | ||
}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for deduping that
describe("source maps", () => { | ||
function buildWithSourceMaps() { | ||
return new Promise((resolve, reject) => { | ||
webpack({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't you re-use runWebpack
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This uses different values for module.rules
, which webpack-merge
can't merge in (it would just try to add rules to the list rather than replacing it entirely).
test/index.test.js
Outdated
sourceMap.should.have.property("sourceRoot", fakeCwd); | ||
// This number needs to be updated if imports.scss or any dependency of that changes. | ||
// Node Sass includes a duplicate entry, Dart Sass does not. | ||
sourceMap.sources.should.have.length(implementation == nodeSass ? 12 : 11); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can get rid of the duplicated dependency. Makes this test easier. Which dependency is duplicated?
} else { | ||
err.message.should.match(/Expected "{"./); | ||
err.message.should.match(/\(line 3, column 1\)/); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the long run it would probably be better to compare these errors with errors that have been snapshotted during createSpec
, but that's ok for now I guess.
test/index.test.js
Outdated
}, { | ||
implementation: merge(nodeSass, { info: "strange-sass\t1.0.0" }) | ||
}, (err) => { | ||
err.message.should.match(/Unknown implementation "strange-sass"\./); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding tests here 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you rename test/node_modules/module to test/node_modules/module.scss? The missing file extension was on purpose.
That's another situation where you were testing import behavior that's being deprecated in LibSass and isn't supported at all in Dart Sass. See sass/libsass#2672.
callbacks. To avoid this overhead, you can use the | ||
[`fibers`](https://www.npmjs.com/package/fibers) package to call asynchronous | ||
importers from the synchronous code path. To enable this, pass the `Fiber` class | ||
to the `fiber` option: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @just-boris; there's a lot of value in providing an easy pure-JS set-up experience, and then letting performance-conscious users focus on that when they need it.
Something to consider, probably after this PR lands, would be adding an option to run Dart Sass in synchronous mode, which is efficient without the use of fibers (at the cost of not supporting async importers or custom functions).
lib/loader.js
Outdated
const implementation = options.implementation || require("node-sass"); | ||
|
||
validateSassVersion(implementation.info); | ||
asyncSassJobQueue = async.queue(implementation.render, threadPoolSize - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe Dart Sass suffers from that problem, but I wanted to keep the code path as similar between the two implementations as possible.
describe("source maps", () => { | ||
function buildWithSourceMaps() { | ||
return new Promise((resolve, reject) => { | ||
webpack({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This uses different values for module.rules
, which webpack-merge
can't merge in (it would just try to add rules to the list rather than replacing it entirely).
Just a gentle nudge: what's next, in terms of getting this PR merged and released? |
Really not wanting to be a pest, but there are some downstream additional changes that this gates. :) Any chance of getting some further review / a potential merge soon? |
node-sass depends on node-gyp, but vulnerability handling by node-gyp team is very slow so we really need to have options to use alternative sass implementations. Please consider to merge this pull request. |
In preparation for #573 Imports without file extensions are deprecated in LibSass and even not supported in DartSass.
In preparation for #573 Imports without file extensions are deprecated in LibSass and even not supported in DartSass.
I've re-added the CSS tests for the node-sass implementation. I've also removed the queue for DartSass. |
Shipped with |
Rather than always loading Node Sass, this now requires users to pass in
either Dart Sass or Node Sass as an option to the loader.
Closes #435