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 all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 59 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,14 @@ Looking for the webpack 1 loader? Check out the [archive/webpack-1 branch](https
npm install sass-loader node-sass webpack --save-dev
```

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

@just-boris just-boris Jul 11, 2018

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
}
}]
}]
}
};
```

### 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
69 changes: 49 additions & 20 deletions lib/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,9 @@ 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.
// This can be removed as soon as node-sass implements a fix for this.
const threadPoolSize = process.env.UV_THREADPOOL_SIZE || 4;
let asyncSassJobQueue = null;
let nodeSassJobQueue = null;

/**
* The sass-loader makes node-sass available to webpack modules.
Expand All @@ -20,19 +17,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 @@ -59,8 +43,9 @@ function sassLoader(content) {
return;
}

// start the actual rendering
asyncSassJobQueue.push(options, (err, result) => {
const render = getRenderFuncFromSassImpl(options.implementation || require("node-sass"));

render(options, (err, result) => {
if (err) {
formatSassError(err, this.resourcePath);
err.file && this.dependency(err.file);
Expand Down Expand Up @@ -92,4 +77,48 @@ function sassLoader(content) {
});
}

/**
* Verifies that the implementation and version of Sass is supported by this loader.
*
* @param {Object} module
* @returns {Function}
*/
function getRenderFuncFromSassImpl(module) {
const info = module.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.");
}
return module.render.bind(module);
} 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.");
}
// There is an issue with node-sass when async custom importers are used
// See https://github.com/sass/node-sass/issues/857#issuecomment-93594360
// We need to use a job queue to make sure that one thread is always available to the UV lib
if (nodeSassJobQueue === null) {
const threadPoolSize = Number(process.env.UV_THREADPOOL_SIZE || 4);

nodeSassJobQueue = async.queue(module.render.bind(module), threadPoolSize - 1);
}

return nodeSassJobQueue.push.bind(nodeSassJobQueue);
}
throw new Error("Unknown Sass implementation \"" + implementation + "\".");
}

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
Original file line number Diff line number Diff line change
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
Loading