Skip to content
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

Merged
merged 6 commits into from Aug 1, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
67 changes: 60 additions & 7 deletions README.md
Expand Up @@ -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
Copy link

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.

Copy link
Contributor Author

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.

```

The sass-loader requires [node-sass](https://github.com/sass/node-sass) and [webpack](https://github.com/webpack)
as [`peerDependency`](https://docs.npmjs.com/files/package.json#peerdependencies). Thus you are able to control the versions accurately.
The sass-loader requires [webpack](https://github.com/webpack) as a
[`peerDependency`](https://docs.npmjs.com/files/package.json#peerdependencies)
and it requires you to install either [Node Sass][] or [Dart Sass][] on your
own. This allows you to control the versions of all your dependencies, and to
choose which Sass implementation to use.

[Node Sass]: https://github.com/sass/node-sass
[Dart Sass]: http://sass-lang.com/dart-sass

<h2 align="center">Examples</h2>

Expand All @@ -48,14 +54,14 @@ module.exports = {
use: [
"style-loader", // creates style nodes from JS strings
"css-loader", // translates CSS into CommonJS
"sass-loader" // compiles Sass to CSS
"sass-loader" // compiles Sass to CSS, using Node Sass by default
]
}]
}
};
```

You can also pass options directly to [node-sass](https://github.com/andrew/node-sass) by specifying an `options` property like this:
You can also pass options directly to [Node Sass][] or [Dart Sass][]:

```js
// webpack.config.js
Expand All @@ -79,7 +85,54 @@ module.exports = {
};
```

See [node-sass](https://github.com/andrew/node-sass) for all available Sass options.
See [the Node Sass documentation](https://github.com/sass/node-sass/blob/master/README.md#options) for all available Sass options.

The special `implementation` option determines which implementation of Sass to
use. It takes either a [Node Sass][] or a [Dart Sass][] module. For example, to
use Dart Sass, you'd pass:

```js
// ...
{
loader: "sass-loader",
options: {
implementation: require("sass")
}
}
// ...
```

Note that when using Dart Sass, **synchronous compilation is twice as fast as
asynchronous compilation** by default, due to the overhead of asynchronous
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:
Copy link
Member

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.

Copy link

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".

Copy link
Contributor Author

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).

Copy link
Member

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.


```js
// webpack.config.js
const Fiber = require('fibers');

module.exports = {
...
module: {
rules: [{
test: /\.scss$/,
use: [{
loader: "style-loader"
}, {
loader: "css-loader"
}, {
loader: "sass-loader",
options: {
implementation: require("sass"),
fiber: Fiber
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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

}
}]
}]
}
};
```

### In production

Expand Down Expand Up @@ -116,7 +169,7 @@ module.exports = {

### Imports

webpack provides an [advanced mechanism to resolve files](https://webpack.js.org/concepts/module-resolution/). The sass-loader uses node-sass' custom importer feature to pass all queries to the webpack resolving engine. Thus you can import your Sass modules from `node_modules`. Just prepend them with a `~` to tell webpack that this is not a relative import:
webpack provides an [advanced mechanism to resolve files](https://webpack.js.org/concepts/module-resolution/). The sass-loader uses Sass's custom importer feature to pass all queries to the webpack resolving engine. Thus you can import your Sass modules from `node_modules`. Just prepend them with a `~` to tell webpack that this is not a relative import:

```css
@import "~bootstrap/dist/css/bootstrap";
Expand Down
53 changes: 40 additions & 13 deletions lib/loader.js
Expand Up @@ -6,6 +6,7 @@ const formatSassError = require("./formatSassError");
const webpackImporter = require("./webpackImporter");
const normalizeOptions = require("./normalizeOptions");
const pify = require("pify");
const semver = require("semver");

// This queue makes sure node-sass leaves one thread available for executing
// fs tasks when running the custom importer code.
Expand All @@ -20,19 +21,6 @@ let asyncSassJobQueue = null;
* @param {string} content
*/
function sassLoader(content) {
if (asyncSassJobQueue === null) {
const sass = require("node-sass");
const sassVersion = /^(\d+)/.exec(require("node-sass/package.json").version).pop();

if (Number(sassVersion) < 4) {
throw new Error(
"The installed version of `node-sass` is not compatible (expected: >= 4, actual: " + sassVersion + ")."
);
}

asyncSassJobQueue = async.queue(sass.render, threadPoolSize - 1);
}

const callback = this.async();
const isSync = typeof callback !== "function";
const self = this;
Expand All @@ -53,6 +41,13 @@ function sassLoader(content) {
addNormalizedDependency
));

if (asyncSassJobQueue === null) {
const implementation = options.implementation || require("node-sass");

validateSassVersion(implementation.info);
asyncSassJobQueue = async.queue(implementation.render, threadPoolSize - 1);
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

}

// Skip empty files, otherwise it will stop webpack, see issue #21
if (options.data.trim() === "") {
callback(null, "");
Expand Down Expand Up @@ -92,4 +87,36 @@ function sassLoader(content) {
});
}

/**
* Verifies that the implementation and version of Sass is supported by this loader.
*
* @param {string} info
*/
function validateSassVersion(info) {
const components = info.split("\t");

if (components.length < 2) {
throw new Error("Unknown Sass implementation \"" + info + "\".");
}

const implementation = components[0];
const version = components[1];

if (!semver.valid(version)) {
throw new Error("Invalid Sass version \"" + version + "\".");
}

if (implementation === "dart-sass") {
if (!semver.satisfies(version, "^1.3.0")) {
throw new Error("Dart Sass version " + version + " is incompatible with ^1.3.0.");
}
} else if (implementation === "node-sass") {
if (!semver.satisfies(version, "^4.0.0")) {
throw new Error("Node Sass version " + version + " is incompatible with ^4.0.0.");
}
} else {
throw new Error("Unknown implementation \"" + implementation + "\".");
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

module.exports = sassLoader;
29 changes: 21 additions & 8 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion package.json
Expand Up @@ -30,7 +30,8 @@
"loader-utils": "^1.0.1",
"lodash.tail": "^4.1.1",
"neo-async": "^2.5.0",
"pify": "^3.0.0"
"pify": "^3.0.0",
"semver": "^5.5.0"
},
"devDependencies": {
"bootstrap-sass": "^3.3.5",
Expand All @@ -44,6 +45,7 @@
"node-sass": "^4.5.0",
"nyc": "^11.0.2",
"raw-loader": "^0.5.1",
"sass": "^1.3.0",
"should": "^11.2.0",
"standard-version": "^4.2.0",
"style-loader": "^0.18.2",
Expand Down