From 55e9b4857db180cad9af980349fad95f07e80c41 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Fri, 25 May 2018 14:08:47 -0400 Subject: [PATCH 1/5] Make this package implementation-agnostic 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 --- README.md | 67 +- lib/loader.js | 53 +- package-lock.json | 29 +- package.json | 4 +- test/index.test.js | 618 ++++++++++-------- test/node_modules/animate.css/animate.css | 3 - test/node_modules/css/some-css-module.css | 3 - test/sass/import-css.sass | 7 - test/sass/import-from-npm-org-pkg.sass | 2 +- test/sass/import-include-paths.sass | 1 - test/sass/imports.sass | 5 +- test/sass/includePath/animate.css/animate.css | 3 - test/sass/spec/dart-sass/.gitignore | 2 + test/sass/spec/node-sass/.gitignore | 2 + test/scss/import-css.scss | 7 - test/scss/import-include-paths.scss | 1 - test/scss/imports.scss | 3 - test/scss/includePath/animate.css/animate.css | 3 - test/scss/spec/dart-sass/.gitignore | 2 + test/scss/spec/node-sass/.gitignore | 2 + test/tools/createSpec.js | 17 +- test/tools/customFunctions.js | 24 +- 22 files changed, 505 insertions(+), 353 deletions(-) delete mode 100644 test/node_modules/animate.css/animate.css delete mode 100644 test/node_modules/css/some-css-module.css delete mode 100644 test/sass/import-css.sass delete mode 100644 test/sass/includePath/animate.css/animate.css create mode 100644 test/sass/spec/dart-sass/.gitignore create mode 100644 test/sass/spec/node-sass/.gitignore delete mode 100644 test/scss/import-css.scss delete mode 100644 test/scss/includePath/animate.css/animate.css create mode 100644 test/scss/spec/dart-sass/.gitignore create mode 100644 test/scss/spec/node-sass/.gitignore diff --git a/README.md b/README.md index b6b42ff5..493494c1 100644 --- a/README.md +++ b/README.md @@ -24,11 +24,17 @@ Looking for the webpack 1 loader? Check out the [archive/webpack-1 branch](https

Install

```bash -npm install sass-loader node-sass webpack --save-dev +npm install sass-loader 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

Examples

@@ -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 @@ -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: + +```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 @@ -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"; diff --git a/lib/loader.js b/lib/loader.js index 531196f3..03b3a524 100644 --- a/lib/loader.js +++ b/lib/loader.js @@ -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. @@ -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; @@ -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); + } + // Skip empty files, otherwise it will stop webpack, see issue #21 if (options.data.trim() === "") { callback(null, ""); @@ -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 + "\"."); + } +} + module.exports = sassLoader; diff --git a/package-lock.json b/package-lock.json index d56a84c5..f5c193af 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1271,7 +1271,7 @@ "json-stringify-safe": "5.0.1", "lodash": "4.17.4", "meow": "3.7.0", - "semver": "5.3.0", + "semver": "5.5.0", "split": "1.0.0", "through2": "2.0.3" } @@ -3728,7 +3728,7 @@ "dev": true, "requires": { "meow": "3.7.0", - "semver": "5.3.0" + "semver": "5.5.0" } }, "gitconfiglocal": { @@ -5436,6 +5436,14 @@ "semver": "5.3.0", "tar": "2.2.1", "which": "1.2.14" + }, + "dependencies": { + "semver": { + "version": "5.3.0", + "resolved": "https://registry.npmjs.org/semver/-/semver-5.3.0.tgz", + "integrity": "sha1-myzl094C0XxgEq0yaqa00M9U+U8=", + "dev": true + } } }, "node-libs-browser": { @@ -5512,7 +5520,7 @@ "requires": { "hosted-git-info": "2.5.0", "is-builtin-module": "1.0.0", - "semver": "5.3.0", + "semver": "5.5.0", "validate-npm-package-license": "3.0.1" } }, @@ -8888,6 +8896,12 @@ "ret": "0.1.15" } }, + "sass": { + "version": "1.5.1", + "resolved": "https://registry.npmjs.org/sass/-/sass-1.5.1.tgz", + "integrity": "sha512-C9s7SFttwy5OnXDs0ZFVv7c659A7GG/0R0VyrQaGXR07cucSJA2E5XN2ZhLF3kklN90aXuPD2rphkNWyeb6y2A==", + "dev": true + }, "sass-graph": { "version": "2.2.4", "resolved": "https://registry.npmjs.org/sass-graph/-/sass-graph-2.2.4.tgz", @@ -8966,10 +8980,9 @@ } }, "semver": { - "version": "5.3.0", - "resolved": "https://registry.npmjs.org/semver/-/semver-5.3.0.tgz", - "integrity": "sha1-myzl094C0XxgEq0yaqa00M9U+U8=", - "dev": true + "version": "5.5.0", + "resolved": "https://registry.npmjs.org/semver/-/semver-5.5.0.tgz", + "integrity": "sha512-4SJ3dm0WAwWy/NVeioZh5AntkdJoWKxHxcmyP622fOkgHa4z3R0TdBJICINyaSDE6uNwVc8gZr+ZinwZAH4xIA==" }, "send": { "version": "0.15.3", @@ -9508,7 +9521,7 @@ "conventional-recommended-bump": "1.0.0", "figures": "1.7.0", "fs-access": "1.0.1", - "semver": "5.3.0", + "semver": "5.5.0", "yargs": "8.0.2" }, "dependencies": { diff --git a/package.json b/package.json index 0cbcb5ed..7e7cedb1 100644 --- a/package.json +++ b/package.json @@ -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", @@ -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", diff --git a/test/index.test.js b/test/index.test.js index a0cf7bd6..5b29a5e6 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -6,6 +6,8 @@ const path = require("path"); const webpack = require("webpack"); const fs = require("fs"); const merge = require("webpack-merge"); +const nodeSass = require("node-sass"); +const dartSass = require("sass"); const customImporter = require("./tools/customImporter.js"); const customFunctions = require("./tools/customFunctions.js"); const pathToSassLoader = require.resolve("../lib/loader.js"); @@ -14,6 +16,7 @@ const sassLoader = require(pathToSassLoader); const mockRequire = require("mock-require"); const CR = /\r/g; +const implementations = [nodeSass, dartSass]; const syntaxStyles = ["scss", "sass"]; const pathToErrorFileNotFound = path.resolve(__dirname, "./scss/error-file-not-found.scss"); const pathToErrorFileNotFound2 = path.resolve(__dirname, "./scss/error-file-not-found-2.scss"); @@ -32,299 +35,374 @@ Object.defineProperty(loaderContextMock, "options", { } }); -syntaxStyles.forEach(ext => { - function execTest(testId, loaderOptions, webpackOptions) { - return new Promise((resolve, reject) => { - const baseConfig = merge({ - entry: path.join(__dirname, ext, testId + "." + ext), - output: { - filename: "bundle." + ext + ".js" - }, - module: { - rules: [{ - test: new RegExp(`\\.${ ext }$`), - use: [ - { loader: "raw-loader" }, - { loader: pathToSassLoader, options: loaderOptions } - ] - }] - } - }, webpackOptions); +implementations.forEach(implementation => { + const implementationName = implementation.info.split("\t")[0]; + describe(implementationName, () => { + syntaxStyles.forEach(ext => { + function execTest(testId, loaderOptions, webpackOptions) { + const bundleName = "bundle." + ext + "." + implementationName + ".js"; + return new Promise((resolve, reject) => { + const baseConfig = merge({ + entry: path.join(__dirname, ext, testId + "." + ext), + output: { + filename: bundleName + } + }, webpackOptions); - runWebpack(baseConfig, (err) => err ? reject(err) : resolve()); - }).then(() => { - const actualCss = readBundle("bundle." + ext + ".js"); - const expectedCss = readCss(ext, testId); + runWebpack(baseConfig, loaderOptions, (err) => err ? reject(err) : resolve()); + }).then(() => { + const actualCss = readBundle(bundleName); + const expectedCss = readCss(ext, testId); - // writing the actual css to output-dir for better debugging - // fs.writeFileSync(path.join(__dirname, "output", `${ testId }.${ ext }.css`), actualCss, "utf8"); - actualCss.should.eql(expectedCss); - }); - } + // writing the actual css to output-dir for better debugging + // fs.writeFileSync(path.join(__dirname, "output", `${ testId }.${ ext }.css`), actualCss, "utf8"); + actualCss.should.eql(expectedCss); + }); + } - describe(`sass-loader (${ ext })`, () => { - describe("basic", () => { - it("should compile simple sass without errors", () => execTest("language")); - }); - describe("imports", () => { - it("should resolve imports correctly", () => execTest("imports")); - // Test for issue: https://github.com/webpack-contrib/sass-loader/issues/32 - it("should pass with multiple imports", () => execTest("multiple-imports")); - // Test for issue: https://github.com/webpack-contrib/sass-loader/issues/73 - it("should resolve imports from other language style correctly", () => execTest("import-other-style")); - // Test for includePath imports - it("should resolve imports from another directory declared by includePaths correctly", () => execTest("import-include-paths", { - includePaths: [path.join(__dirname, ext, "includePath")] - })); - it("should not resolve CSS imports", () => execTest("import-css")); - it("should compile bootstrap-sass without errors", () => execTest("bootstrap-sass")); - it("should correctly import scoped npm packages", () => execTest("import-from-npm-org-pkg")); - it("should resolve aliases", () => execTest("import-alias", {}, { - resolve: { - alias: { - "path-to-alias": path.join(__dirname, ext, "another", "alias." + ext) - } - } - })); - }); - describe("custom importers", () => { - it("should use custom importer", () => execTest("custom-importer", { - importer: customImporter - })); - }); - describe("custom functions", () => { - it("should expose custom functions", () => execTest("custom-functions", { - functions: customFunctions - })); - }); - describe("prepending data", () => { - it("should extend the data-option if present", () => execTest("prepending-data", { - data: "$prepended-data: hotpink;" - })); - }); - // See https://github.com/webpack-contrib/sass-loader/issues/21 - describe("empty files", () => { - it("should compile without errors", () => execTest("empty")); + describe(`sass-loader (${ ext })`, () => { + describe("basic", () => { + it("should compile simple sass without errors", () => execTest("language")); + }); + describe("imports", () => { + it("should resolve imports correctly", () => execTest("imports")); + // Test for issue: https://github.com/webpack-contrib/sass-loader/issues/32 + it("should pass with multiple imports", () => execTest("multiple-imports")); + // Test for issue: https://github.com/webpack-contrib/sass-loader/issues/73 + it("should resolve imports from other language style correctly", () => execTest("import-other-style")); + // Test for includePath imports + it("should resolve imports from another directory declared by includePaths correctly", () => execTest("import-include-paths", { + includePaths: [path.join(__dirname, ext, "includePath")] + })); + it("should compile bootstrap-sass without errors", () => execTest("bootstrap-sass")); + it("should correctly import scoped npm packages", () => execTest("import-from-npm-org-pkg")); + it("should resolve aliases", () => execTest("import-alias", {}, { + resolve: { + alias: { + "path-to-alias": path.join(__dirname, ext, "another", "alias." + ext) + } + } + })); + }); + describe("custom importers", () => { + it("should use custom importer", () => execTest("custom-importer", { + importer: customImporter + })); + }); + describe("custom functions", () => { + it("should expose custom functions", () => execTest("custom-functions", { + functions: customFunctions(implementation) + })); + }); + describe("prepending data", () => { + it("should extend the data-option if present", () => execTest("prepending-data", { + data: "$prepended-data: hotpink" + (ext == "sass" ? "\n" : ";") + })); + }); + // See https://github.com/webpack-contrib/sass-loader/issues/21 + describe("empty files", () => { + it("should compile without errors", () => execTest("empty")); + }); + }); }); - }); -}); -describe("sass-loader", () => { - describe("multiple compilations", () => { - it("should not interfere with each other", () => - new Promise((resolve, reject) => { - runWebpack({ - entry: { - b: path.join(__dirname, "scss", "multipleCompilations", "b.scss"), - c: path.join(__dirname, "scss", "multipleCompilations", "c.scss"), - a: path.join(__dirname, "scss", "multipleCompilations", "a.scss"), - d: path.join(__dirname, "scss", "multipleCompilations", "d.scss"), - e: path.join(__dirname, "scss", "multipleCompilations", "e.scss"), - f: path.join(__dirname, "scss", "multipleCompilations", "f.scss"), - g: path.join(__dirname, "scss", "multipleCompilations", "g.scss"), - h: path.join(__dirname, "scss", "multipleCompilations", "h.scss") - }, - output: { - filename: "bundle.multiple-compilations.[name].js" - }, - module: { - rules: [{ - test: /\.scss$/, - use: [ - { loader: "raw-loader" }, - // We're specifying an empty options object because otherwise, webpack creates a new object for every loader invocation - // Since we want to ensure that our loader is not tampering with the option object, we are triggering webpack to re-use the options object - // @see https://github.com/webpack-contrib/sass-loader/issues/368#issuecomment-278330164 - { loader: pathToSassLoader, options: {} } - ] - }] - } - }, err => err ? reject(err) : resolve()); - }) - .then(() => { - const expectedCss = readCss("scss", "imports"); - const a = readBundle("bundle.multiple-compilations.a.js"); - const b = readBundle("bundle.multiple-compilations.b.js"); - const c = readBundle("bundle.multiple-compilations.c.js"); - const d = readBundle("bundle.multiple-compilations.d.js"); - const e = readBundle("bundle.multiple-compilations.e.js"); - const f = readBundle("bundle.multiple-compilations.f.js"); - const g = readBundle("bundle.multiple-compilations.g.js"); - const h = readBundle("bundle.multiple-compilations.h.js"); + describe("sass-loader", () => { + describe("multiple compilations", () => { + it("should not interfere with each other", () => + new Promise((resolve, reject) => { + runWebpack({ + entry: { + b: path.join(__dirname, "scss", "multipleCompilations", "b.scss"), + c: path.join(__dirname, "scss", "multipleCompilations", "c.scss"), + a: path.join(__dirname, "scss", "multipleCompilations", "a.scss"), + d: path.join(__dirname, "scss", "multipleCompilations", "d.scss"), + e: path.join(__dirname, "scss", "multipleCompilations", "e.scss"), + f: path.join(__dirname, "scss", "multipleCompilations", "f.scss"), + g: path.join(__dirname, "scss", "multipleCompilations", "g.scss"), + h: path.join(__dirname, "scss", "multipleCompilations", "h.scss") + }, + output: { + filename: "bundle.multiple-compilations.[name].js" + } + }, {}, err => err ? reject(err) : resolve()); + }) + .then(() => { + const expectedCss = readCss("scss", "imports"); + const a = readBundle("bundle.multiple-compilations.a.js"); + const b = readBundle("bundle.multiple-compilations.b.js"); + const c = readBundle("bundle.multiple-compilations.c.js"); + const d = readBundle("bundle.multiple-compilations.d.js"); + const e = readBundle("bundle.multiple-compilations.e.js"); + const f = readBundle("bundle.multiple-compilations.f.js"); + const g = readBundle("bundle.multiple-compilations.g.js"); + const h = readBundle("bundle.multiple-compilations.h.js"); - a.should.equal(expectedCss); - b.should.equal(expectedCss); - c.should.equal(expectedCss); - d.should.equal(expectedCss); - e.should.equal(expectedCss); - f.should.equal(expectedCss); - g.should.equal(expectedCss); - h.should.equal(expectedCss); - }) - ); - }); - describe("source maps", () => { - function buildWithSourceMaps() { - return new Promise((resolve, reject) => { - runWebpack({ - entry: path.join(__dirname, "scss", "imports.scss"), - output: { - filename: "bundle.source-maps.js" - }, - devtool: "source-map", - module: { - rules: [{ - test: /\.scss$/, - use: [ - { loader: testLoader.filename }, - { - loader: pathToSassLoader, options: { - sourceMap: true - } - } - ] - }] - } - }, err => err ? reject(err) : resolve()); + a.should.equal(expectedCss); + b.should.equal(expectedCss); + c.should.equal(expectedCss); + d.should.equal(expectedCss); + e.should.equal(expectedCss); + f.should.equal(expectedCss); + g.should.equal(expectedCss); + h.should.equal(expectedCss); + }) + ); }); - } + describe("source maps", () => { + function buildWithSourceMaps() { + return new Promise((resolve, reject) => { + webpack({ + entry: path.join(__dirname, "scss", "imports.scss"), + mode: "development", + output: { + path: path.join(__dirname, "output"), + filename: "bundle.source-maps.js", + libraryTarget: "commonjs2" + }, + devtool: "source-map", + module: { + rules: [{ + test: /\.scss$/, + use: [ + { loader: testLoader.filename }, + { + loader: pathToSassLoader, options: { + implementation, + sourceMap: true + } + } + ] + }] + } + }, (webpackErr, stats) => { + const err = webpackErr || + (stats.hasErrors() && stats.compilation.errors[0]) || + (stats.hasWarnings() && stats.compilation.warnings[0]); + + if (err) { + reject(err); + } else { + resolve(); + } + }); + }); + } - it("should compile without errors", () => buildWithSourceMaps()); - it("should produce a valid source map", () => { - const cwdGetter = process.cwd; - const fakeCwd = path.join(__dirname, "scss"); + it("should compile without errors", () => buildWithSourceMaps()); + it("should produce a valid source map", () => { + const cwdGetter = process.cwd; + const fakeCwd = path.join(__dirname, "scss"); - process.cwd = function () { - return fakeCwd; - }; + process.cwd = function () { + return fakeCwd; + }; - return buildWithSourceMaps() - .then(() => { - const sourceMap = testLoader.sourceMap; + return buildWithSourceMaps() + .then(() => { + const sourceMap = testLoader.sourceMap; - sourceMap.should.not.have.property("file"); - sourceMap.should.have.property("sourceRoot", fakeCwd); - // This number needs to be updated if imports.scss or any dependency of that changes - sourceMap.sources.should.have.length(13); - sourceMap.sources.forEach(sourcePath => - fs.existsSync(path.resolve(sourceMap.sourceRoot, sourcePath)) - ); + sourceMap.should.not.have.property("file"); + 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); + sourceMap.sources.forEach(sourcePath => + fs.existsSync(path.resolve(sourceMap.sourceRoot, sourcePath)) + ); - process.cwd = cwdGetter; + process.cwd = cwdGetter; + }); }); - }); - }); - describe("errors", () => { - it("should throw an error in synchronous loader environments", () => { - try { - sassLoader.call(Object.create(loaderContextMock), ""); - } catch (err) { - // check for file excerpt - err.message.should.equal("Synchronous compilation is not supported anymore. See https://github.com/webpack-contrib/sass-loader/issues/333"); - } - }); - it("should output understandable errors in entry files", (done) => { - runWebpack({ - entry: pathToSassLoader + "!" + pathToErrorFile - }, (err) => { - err.message.should.match(/Property "some-value" must be followed by a ':'/); - err.message.should.match(/\(line 2, column 5\)/); - err.message.indexOf(pathToErrorFile).should.not.equal(-1); - done(); }); - }); - it("should output understandable errors of imported files", (done) => { - runWebpack({ - entry: pathToSassLoader + "!" + pathToErrorImport - }, (err) => { - // check for file excerpt - err.message.should.match(/Property "some-value" must be followed by a ':'/); - err.message.should.match(/\(line 2, column 5\)/); - err.message.indexOf(pathToErrorFile).should.not.equal(-1); - done(); - }); - }); - it("should output understandable errors when a file could not be found", (done) => { - runWebpack({ - entry: pathToSassLoader + "!" + pathToErrorFileNotFound - }, (err) => { - err.message.should.match(/@import "does-not-exist";/); - err.message.should.match(/File to import not found or unreadable: does-not-exist/); - err.message.should.match(/\(line 1, column 1\)/); - err.message.indexOf(pathToErrorFileNotFound).should.not.equal(-1); - done(); - }); - }); - it("should not auto-resolve imports with explicit file names", (done) => { - runWebpack({ - entry: pathToSassLoader + "!" + pathToErrorFileNotFound2 - }, (err) => { - err.message.should.match(/@import "\.\/another\/_module\.scss";/); - err.message.should.match(/File to import not found or unreadable: \.\/another\/_module\.scss/); - err.message.should.match(/\(line 1, column 1\)/); - err.message.indexOf(pathToErrorFileNotFound2).should.not.equal(-1); - done(); - }); - }); - it("should not swallow errors when trying to load node-sass", (done) => { - mockRequire.reRequire(pathToSassLoader); - const module = require("module"); - const originalResolve = module._resolveFilename; + describe("errors", () => { + it("should throw an error in synchronous loader environments", () => { + try { + sassLoader.call(Object.create(loaderContextMock), ""); + } catch (err) { + // check for file excerpt + err.message.should.equal("Synchronous compilation is not supported anymore. See https://github.com/webpack-contrib/sass-loader/issues/333"); + } + }); + it("should output understandable errors in entry files", (done) => { + runWebpack({ + entry: pathToErrorFile + }, {}, (err) => { + if (implementation === nodeSass) { + err.message.should.match(/Property "some-value" must be followed by a ':'/); + err.message.should.match(/\(line 2, column 5\)/); + } else { + err.message.should.match(/Expected "{"./); + err.message.should.match(/\(line 3, column 1\)/); + } + err.message.indexOf(pathToErrorFile).should.not.equal(-1); + done(); + }); + }); + it("should output understandable errors of imported files", (done) => { + runWebpack({ + entry: pathToErrorImport + }, {}, (err) => { + // check for file excerpt + if (implementation == nodeSass) { + err.message.should.match(/Property "some-value" must be followed by a ':'/); + err.message.should.match(/\(line 2, column 5\)/); + } else { + err.message.should.match(/Expected "{"./); + err.message.should.match(/\(line 3, column 1\)/); + } + err.message.indexOf(pathToErrorFile).should.not.equal(-1); + done(); + }); + }); + it("should output understandable errors when a file could not be found", (done) => { + runWebpack({ + entry: pathToErrorFileNotFound + }, {}, (err) => { + err.message.should.match(/@import "does-not-exist";/); + if (implementation == nodeSass) { + err.message.should.match(/File to import not found or unreadable: does-not-exist/); + err.message.should.match(/\(line 1, column 1\)/); + } else { + err.message.should.match(/Can't find stylesheet to import\./); + err.message.should.match(/\(line 1, column 9\)/); + } + err.message.indexOf(pathToErrorFileNotFound).should.not.equal(-1); + done(); + }); + }); + it("should not auto-resolve imports with explicit file names", (done) => { + runWebpack({ + entry: pathToErrorFileNotFound2 + }, {}, (err) => { + err.message.should.match(/@import "\.\/another\/_module\.scss";/); + if (implementation == nodeSass) { + err.message.should.match(/File to import not found or unreadable: \.\/another\/_module\.scss/); + err.message.should.match(/\(line 1, column 1\)/); + } else { + err.message.should.match(/Can't find stylesheet to import\./); + err.message.should.match(/\(line 1, column 9\)/); + } + err.message.indexOf(pathToErrorFileNotFound2).should.not.equal(-1); + done(); + }); + }); + it("should not swallow errors when trying to load node-sass", (done) => { + mockRequire.reRequire(pathToSassLoader); + const module = require("module"); + const originalResolve = module._resolveFilename; - module._resolveFilename = function (filename) { - if (!filename.match(/node-sass/)) { - return originalResolve.apply(this, arguments); - } - const err = new Error("Some error"); + module._resolveFilename = function (filename) { + if (!filename.match(/node-sass/)) { + return originalResolve.apply(this, arguments); + } + const err = new Error("Some error"); - err.code = "MODULE_NOT_FOUND"; - throw err; - }; - runWebpack({ - entry: pathToSassLoader + "!" + pathToErrorFile - }, (err) => { - module._resolveFilename = originalResolve; - mockRequire.reRequire("node-sass"); - err.message.should.match(/Some error/); - done(); - }); - }); - it("should output a message when `node-sass` is an incompatible version", (done) => { - mockRequire.reRequire(pathToSassLoader); - mockRequire("node-sass/package.json", { version: "3.0.0" }); - runWebpack({ - entry: pathToSassLoader + "!" + pathToErrorFile - }, (err) => { - mockRequire.stop("node-sass"); - err.message.should.match(/The installed version of `node-sass` is not compatible/); - done(); + err.code = "MODULE_NOT_FOUND"; + throw err; + }; + runWebpack({ + entry: pathToSassLoader + "!" + pathToErrorFile + }, { implementation: null }, (err) => { + module._resolveFilename = originalResolve; + mockRequire.reRequire("node-sass"); + err.message.should.match(/Some error/); + done(); + }); + }); + it("should output a message when the Sass info is unparseable", (done) => { + mockRequire.reRequire(pathToSassLoader); + runWebpack({ + entry: pathToErrorFile + }, { + implementation: merge(nodeSass, { info: "asdfj" }) + }, (err) => { + err.message.should.match(/Unknown Sass implementation "asdfj"\./); + done(); + }); + }); + it("should output a message when the Sass version is unparseable", (done) => { + mockRequire.reRequire(pathToSassLoader); + runWebpack({ + entry: pathToErrorFile + }, { + implementation: merge(nodeSass, { info: "node-sass\t1" }) + }, (err) => { + err.message.should.match(/Invalid Sass version "1"\./); + done(); + }); + }); + it("should output a message when Node Sass is an incompatible version", (done) => { + mockRequire.reRequire(pathToSassLoader); + runWebpack({ + entry: pathToErrorFile + }, { + implementation: merge(nodeSass, { info: "node-sass\t3.0.0" }) + }, (err) => { + err.message.should.match(/Node Sass version 3\.0\.0 is incompatible with \^4\.0\.0\./); + done(); + }); + }); + it("should output a message when Dart Sass is an incompatible version", (done) => { + mockRequire.reRequire(pathToSassLoader); + runWebpack({ + entry: pathToErrorFile + }, { + implementation: merge(nodeSass, { info: "dart-sass\t1.2.0" }) + }, (err) => { + err.message.should.match(/Dart Sass version 1\.2\.0 is incompatible with \^1\.3\.0\./); + done(); + }); + }); + it("should output a message for an unknown implementation", (done) => { + mockRequire.reRequire(pathToSassLoader); + runWebpack({ + entry: pathToErrorFile + }, { + implementation: merge(nodeSass, { info: "strange-sass\t1.0.0" }) + }, (err) => { + err.message.should.match(/Unknown implementation "strange-sass"\./); + done(); + }); + }); }); }); - }); -}); - -function readCss(ext, id) { - return fs.readFileSync(path.join(__dirname, ext, "spec", id + ".css"), "utf8").replace(CR, ""); -} -function runWebpack(baseConfig, done) { - const webpackConfig = merge({ - mode: "development", - output: { - path: path.join(__dirname, "output"), - filename: "bundle.js", - libraryTarget: "commonjs2" + function readCss(ext, id) { + return fs.readFileSync(path.join(__dirname, ext, "spec", implementationName, id + ".css"), "utf8").replace(CR, ""); } - }, baseConfig); - webpack(webpackConfig, (webpackErr, stats) => { - const err = webpackErr || - (stats.hasErrors() && stats.compilation.errors[0]) || - (stats.hasWarnings() && stats.compilation.warnings[0]); + function runWebpack(baseConfig, loaderOptions, done) { + const webpackConfig = merge({ + mode: "development", + output: { + path: path.join(__dirname, "output"), + filename: "bundle.js", + libraryTarget: "commonjs2" + }, + module: { + rules: [{ + test: /\.s[ac]ss$/, + use: [ + { loader: "raw-loader" }, + { + loader: pathToSassLoader, + options: merge({ implementation }, loaderOptions) + } + ] + }] + } + }, baseConfig); + + webpack(webpackConfig, (webpackErr, stats) => { + const err = webpackErr || + (stats.hasErrors() && stats.compilation.errors[0]) || + (stats.hasWarnings() && stats.compilation.warnings[0]); - done(err || null); + done(err || null); + }); + } }); -} +}); function readBundle(filename) { delete require.cache[path.resolve(__dirname, `./output/${ filename }`)]; diff --git a/test/node_modules/animate.css/animate.css b/test/node_modules/animate.css/animate.css deleted file mode 100644 index 6ff1c8d0..00000000 --- a/test/node_modules/animate.css/animate.css +++ /dev/null @@ -1,3 +0,0 @@ -.animate-css { - background: hotpink; -} diff --git a/test/node_modules/css/some-css-module.css b/test/node_modules/css/some-css-module.css deleted file mode 100644 index ba881dc7..00000000 --- a/test/node_modules/css/some-css-module.css +++ /dev/null @@ -1,3 +0,0 @@ -.some-css-module { - background: hotpink; -} diff --git a/test/sass/import-css.sass b/test/sass/import-css.sass deleted file mode 100644 index 1bbb12d8..00000000 --- a/test/sass/import-css.sass +++ /dev/null @@ -1,7 +0,0 @@ -// Special behavior of node-sass/libsass with CSS-files -// 1. CSS-files are not included, but linked with @import url(path/to/css) -@import ~css/some-css-module.css -// 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 diff --git a/test/sass/import-from-npm-org-pkg.sass b/test/sass/import-from-npm-org-pkg.sass index 43c22271..9fc088eb 100644 --- a/test/sass/import-from-npm-org-pkg.sass +++ b/test/sass/import-from-npm-org-pkg.sass @@ -4,4 +4,4 @@ @import ~@org/bar/foo .foo - background: #000; + background: #000 diff --git a/test/sass/import-include-paths.sass b/test/sass/import-include-paths.sass index d6b7c510..9cbcb635 100644 --- a/test/sass/import-include-paths.sass +++ b/test/sass/import-include-paths.sass @@ -1,3 +1,2 @@ @import include-path-module @import underscore-include-path-module -@import animate.css/animate diff --git a/test/sass/imports.sass b/test/sass/imports.sass index fc93a369..7bee8c0c 100644 --- a/test/sass/imports.sass +++ b/test/sass/imports.sass @@ -10,11 +10,8 @@ // @see https://github.com/webpack-contrib/sass-loader/issues/167 /* @import ~sass/some.module */ @import ~sass/some.module -// @see https://github.com/webpack-contrib/sass-loader/issues/360 -/* @import ~animate.css/animate */ -@import ~animate.css/animate /* @import url(http://example.com/something/from/the/interwebs); */ -@import url(http://example.com/something/from/the/interwebs); +@import url(http://example.com/something/from/the/interwebs) /* scoped import @import language */ .scoped-imporr @import language diff --git a/test/sass/includePath/animate.css/animate.css b/test/sass/includePath/animate.css/animate.css deleted file mode 100644 index 6ff1c8d0..00000000 --- a/test/sass/includePath/animate.css/animate.css +++ /dev/null @@ -1,3 +0,0 @@ -.animate-css { - background: hotpink; -} diff --git a/test/sass/spec/dart-sass/.gitignore b/test/sass/spec/dart-sass/.gitignore new file mode 100644 index 00000000..db8b4be7 --- /dev/null +++ b/test/sass/spec/dart-sass/.gitignore @@ -0,0 +1,2 @@ +# Will be populated by npm pretest +*.css diff --git a/test/sass/spec/node-sass/.gitignore b/test/sass/spec/node-sass/.gitignore new file mode 100644 index 00000000..db8b4be7 --- /dev/null +++ b/test/sass/spec/node-sass/.gitignore @@ -0,0 +1,2 @@ +# Will be populated by npm pretest +*.css diff --git a/test/scss/import-css.scss b/test/scss/import-css.scss deleted file mode 100644 index 0a9345fc..00000000 --- a/test/scss/import-css.scss +++ /dev/null @@ -1,7 +0,0 @@ -// Special behavior of node-sass/libsass with CSS-files -// 1. CSS-files are not included, but linked with @import url(path/to/css) -@import "~css/some-css-module.css"; -// 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"; diff --git a/test/scss/import-include-paths.scss b/test/scss/import-include-paths.scss index 23adb601..15b70814 100644 --- a/test/scss/import-include-paths.scss +++ b/test/scss/import-include-paths.scss @@ -1,3 +1,2 @@ @import "include-path-module"; @import "underscore-include-path-module"; -@import "animate.css/animate"; diff --git a/test/scss/imports.scss b/test/scss/imports.scss index bd6736c9..227bdbc6 100644 --- a/test/scss/imports.scss +++ b/test/scss/imports.scss @@ -12,9 +12,6 @@ // @see https://github.com/webpack-contrib/sass-loader/issues/167 /* @import "~scss/some.module"; */ @import "~scss/some.module"; -// @see https://github.com/webpack-contrib/sass-loader/issues/360 -/* @import "~animate.css/animate"; */ -@import "~animate.css/animate"; /* @import url(http://example.com/something/from/the/interwebs); */ @import url(http://example.com/something/from/the/interwebs); /* scoped import @import "language"; */ diff --git a/test/scss/includePath/animate.css/animate.css b/test/scss/includePath/animate.css/animate.css deleted file mode 100644 index 6ff1c8d0..00000000 --- a/test/scss/includePath/animate.css/animate.css +++ /dev/null @@ -1,3 +0,0 @@ -.animate-css { - background: hotpink; -} diff --git a/test/scss/spec/dart-sass/.gitignore b/test/scss/spec/dart-sass/.gitignore new file mode 100644 index 00000000..db8b4be7 --- /dev/null +++ b/test/scss/spec/dart-sass/.gitignore @@ -0,0 +1,2 @@ +# Will be populated by npm pretest +*.css diff --git a/test/scss/spec/node-sass/.gitignore b/test/scss/spec/node-sass/.gitignore new file mode 100644 index 00000000..db8b4be7 --- /dev/null +++ b/test/scss/spec/node-sass/.gitignore @@ -0,0 +1,2 @@ +# Will be populated by npm pretest +*.css diff --git a/test/tools/createSpec.js b/test/tools/createSpec.js index 84a95a53..38daf95f 100644 --- a/test/tools/createSpec.js +++ b/test/tools/createSpec.js @@ -1,12 +1,14 @@ "use strict"; -const sass = require("node-sass"); +const nodeSass = require("node-sass"); +const dartSass = require("sass"); const os = require("os"); const fs = require("fs"); const path = require("path"); const customImporter = require("./customImporter.js"); const customFunctions = require("./customFunctions.js"); +const implementations = [nodeSass, dartSass]; const testFolder = path.resolve(__dirname, "../"); const error = "error"; @@ -40,7 +42,6 @@ function createSpec(ext) { file: url }; }, - functions: customFunctions, includePaths: [ path.join(testFolder, ext, "another"), path.join(testFolder, ext, "includePath") @@ -48,15 +49,19 @@ function createSpec(ext) { }; if (/prepending-data/.test(fileName)) { - sassOptions.data = "$prepended-data: hotpink;" + os.EOL + fs.readFileSync(fileName, "utf8"); sassOptions.indentedSyntax = /\.sass$/.test(fileName); + sassOptions.data = "$prepended-data: hotpink" + (sassOptions.indentedSyntax ? "\n" : ";") + + os.EOL + fs.readFileSync(fileName, "utf8"); } else { sassOptions.file = fileName; } - const css = sass.renderSync(sassOptions).css; - - fs.writeFileSync(path.join(basePath, "spec", fileWithoutExt + ".css"), css, "utf8"); + implementations.forEach(implementation => { + sassOptions.functions = customFunctions(implementation); + const name = implementation.info.split("\t")[0]; + const css = implementation.renderSync(sassOptions).css; + fs.writeFileSync(path.join(basePath, "spec", name, fileWithoutExt + ".css"), css, "utf8"); + }); }); } diff --git a/test/tools/customFunctions.js b/test/tools/customFunctions.js index 968321c7..57c9ea90 100644 --- a/test/tools/customFunctions.js +++ b/test/tools/customFunctions.js @@ -1,18 +1,18 @@ "use strict"; -const sass = require("node-sass"); +module.exports = function(implementation) { + return { + "headings($from: 0, $to: 6)"(from, to) { + const f = from.getValue(); + const t = to.getValue(); + const list = new implementation.types.List(t - f + 1); + let i; -module.exports = { - "headings($from: 0, $to: 6)"(from, to) { - const f = from.getValue(); - const t = to.getValue(); - const list = new sass.types.List(t - f + 1); - let i; + for (i = f; i <= t; i++) { + list.setValue(i - f, new implementation.types.String("h" + i)); + } - for (i = f; i <= t; i++) { - list.setValue(i - f, new sass.types.String("h" + i)); + return list; } - - return list; - } + }; }; From 3d240896b6b3266426b3c8a75e9f095c33d12ce8 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Mon, 4 Jun 2018 12:14:45 -0400 Subject: [PATCH 2/5] Give new modules file extensions Importing stylesheets without extensions is non-standard behavior that's not supported in Dart Sass (see sass/libsass#2672). --- test/index.test.js | 12 ++++++++++++ test/node_modules/{module => module.scss} | 0 .../node_modules/{other-module => other-module.scss} | 0 3 files changed, 12 insertions(+) rename test/node_modules/{module => module.scss} (100%) rename test/node_modules/{other-module => other-module.scss} (100%) diff --git a/test/index.test.js b/test/index.test.js index 5b29a5e6..ba1e7106 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -159,6 +159,12 @@ implementations.forEach(implementation => { filename: "bundle.source-maps.js", libraryTarget: "commonjs2" }, + resolve: { + alias: { + module: 'module.scss', + 'other-module': 'other-module.scss' + } + }, devtool: "source-map", module: { rules: [{ @@ -390,6 +396,12 @@ implementations.forEach(implementation => { } ] }] + }, + resolve: { + alias: { + module: 'module.scss', + 'other-module': 'other-module.scss' + } } }, baseConfig); diff --git a/test/node_modules/module b/test/node_modules/module.scss similarity index 100% rename from test/node_modules/module rename to test/node_modules/module.scss diff --git a/test/node_modules/other-module b/test/node_modules/other-module.scss similarity index 100% rename from test/node_modules/other-module rename to test/node_modules/other-module.scss From 05d20e6852df1a351347a4cce3fc3480bfd1955e Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Mon, 9 Jul 2018 17:48:27 -0700 Subject: [PATCH 3/5] Install Node Sass in the example --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 493494c1..32771086 100644 --- a/README.md +++ b/README.md @@ -24,7 +24,7 @@ Looking for the webpack 1 loader? Check out the [archive/webpack-1 branch](https

Install

```bash -npm install sass-loader sass webpack --save-dev +npm install sass-loader node-sass webpack --save-dev ``` The sass-loader requires [webpack](https://github.com/webpack) as a From 8f8081b3ff24c219b432190adb4ce5f7769d374a Mon Sep 17 00:00:00 2001 From: Johannes Ewald Date: Wed, 1 Aug 2018 16:24:20 +0200 Subject: [PATCH 4/5] Re-add CSS tests for node-sass implementation --- lib/loader.js | 4 ++-- test/index.test.js | 29 ++++++++++++++--------- test/node_modules/css/some-css-module.css | 3 +++ test/sass/import-css.sass | 7 ++++++ test/scss/import-css.scss | 7 ++++++ test/tools/createSpec.js | 9 +++++++ test/tools/customFunctions.js | 2 +- 7 files changed, 47 insertions(+), 14 deletions(-) create mode 100644 test/node_modules/css/some-css-module.css create mode 100644 test/sass/import-css.sass create mode 100644 test/scss/import-css.scss diff --git a/lib/loader.js b/lib/loader.js index 03b3a524..850c67c6 100644 --- a/lib/loader.js +++ b/lib/loader.js @@ -11,7 +11,7 @@ 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; +const threadPoolSize = Number(process.env.UV_THREADPOOL_SIZE || 4); let asyncSassJobQueue = null; /** @@ -115,7 +115,7 @@ function validateSassVersion(info) { throw new Error("Node Sass version " + version + " is incompatible with ^4.0.0."); } } else { - throw new Error("Unknown implementation \"" + implementation + "\"."); + throw new Error("Unknown Sass implementation \"" + implementation + "\"."); } } diff --git a/test/index.test.js b/test/index.test.js index ba1e7106..23b12add 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -37,10 +37,12 @@ Object.defineProperty(loaderContextMock, "options", { implementations.forEach(implementation => { const implementationName = implementation.info.split("\t")[0]; + describe(implementationName, () => { syntaxStyles.forEach(ext => { function execTest(testId, loaderOptions, webpackOptions) { const bundleName = "bundle." + ext + "." + implementationName + ".js"; + return new Promise((resolve, reject) => { const baseConfig = merge({ entry: path.join(__dirname, ext, testId + "." + ext), @@ -74,6 +76,11 @@ implementations.forEach(implementation => { it("should resolve imports from another directory declared by includePaths correctly", () => execTest("import-include-paths", { includePaths: [path.join(__dirname, ext, "includePath")] })); + // Legacy support for CSS imports with node-sass + // See discussion https://github.com/webpack-contrib/sass-loader/pull/573/files?#r199109203 + if (implementation === nodeSass) { + it("should not resolve CSS imports", () => execTest("import-css")); + } it("should compile bootstrap-sass without errors", () => execTest("bootstrap-sass")); it("should correctly import scoped npm packages", () => execTest("import-from-npm-org-pkg")); it("should resolve aliases", () => execTest("import-alias", {}, { @@ -96,7 +103,7 @@ implementations.forEach(implementation => { }); describe("prepending data", () => { it("should extend the data-option if present", () => execTest("prepending-data", { - data: "$prepended-data: hotpink" + (ext == "sass" ? "\n" : ";") + data: "$prepended-data: hotpink" + (ext === "sass" ? "\n" : ";") })); }); // See https://github.com/webpack-contrib/sass-loader/issues/21 @@ -161,8 +168,8 @@ implementations.forEach(implementation => { }, resolve: { alias: { - module: 'module.scss', - 'other-module': 'other-module.scss' + module: "module.scss", + "other-module": "other-module.scss" } }, devtool: "source-map", @@ -211,7 +218,7 @@ implementations.forEach(implementation => { 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); + sourceMap.sources.should.have.length(implementation === nodeSass ? 12 : 11); sourceMap.sources.forEach(sourcePath => fs.existsSync(path.resolve(sourceMap.sourceRoot, sourcePath)) ); @@ -249,7 +256,7 @@ implementations.forEach(implementation => { entry: pathToErrorImport }, {}, (err) => { // check for file excerpt - if (implementation == nodeSass) { + if (implementation === nodeSass) { err.message.should.match(/Property "some-value" must be followed by a ':'/); err.message.should.match(/\(line 2, column 5\)/); } else { @@ -265,7 +272,7 @@ implementations.forEach(implementation => { entry: pathToErrorFileNotFound }, {}, (err) => { err.message.should.match(/@import "does-not-exist";/); - if (implementation == nodeSass) { + if (implementation === nodeSass) { err.message.should.match(/File to import not found or unreadable: does-not-exist/); err.message.should.match(/\(line 1, column 1\)/); } else { @@ -281,7 +288,7 @@ implementations.forEach(implementation => { entry: pathToErrorFileNotFound2 }, {}, (err) => { err.message.should.match(/@import "\.\/another\/_module\.scss";/); - if (implementation == nodeSass) { + if (implementation === nodeSass) { err.message.should.match(/File to import not found or unreadable: \.\/another\/_module\.scss/); err.message.should.match(/\(line 1, column 1\)/); } else { @@ -359,14 +366,14 @@ implementations.forEach(implementation => { done(); }); }); - it("should output a message for an unknown implementation", (done) => { + it("should output a message for an unknown sass implementation", (done) => { mockRequire.reRequire(pathToSassLoader); runWebpack({ entry: pathToErrorFile }, { implementation: merge(nodeSass, { info: "strange-sass\t1.0.0" }) }, (err) => { - err.message.should.match(/Unknown implementation "strange-sass"\./); + err.message.should.match(/Unknown Sass implementation "strange-sass"\./); done(); }); }); @@ -399,8 +406,8 @@ implementations.forEach(implementation => { }, resolve: { alias: { - module: 'module.scss', - 'other-module': 'other-module.scss' + module: "module.scss", + "other-module": "other-module.scss" } } }, baseConfig); diff --git a/test/node_modules/css/some-css-module.css b/test/node_modules/css/some-css-module.css new file mode 100644 index 00000000..ba881dc7 --- /dev/null +++ b/test/node_modules/css/some-css-module.css @@ -0,0 +1,3 @@ +.some-css-module { + background: hotpink; +} diff --git a/test/sass/import-css.sass b/test/sass/import-css.sass new file mode 100644 index 00000000..1bbb12d8 --- /dev/null +++ b/test/sass/import-css.sass @@ -0,0 +1,7 @@ +// Special behavior of node-sass/libsass with CSS-files +// 1. CSS-files are not included, but linked with @import url(path/to/css) +@import ~css/some-css-module.css +// 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 diff --git a/test/scss/import-css.scss b/test/scss/import-css.scss new file mode 100644 index 00000000..0a9345fc --- /dev/null +++ b/test/scss/import-css.scss @@ -0,0 +1,7 @@ +// Special behavior of node-sass/libsass with CSS-files +// 1. CSS-files are not included, but linked with @import url(path/to/css) +@import "~css/some-css-module.css"; +// 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"; diff --git a/test/tools/createSpec.js b/test/tools/createSpec.js index 38daf95f..74bda3da 100644 --- a/test/tools/createSpec.js +++ b/test/tools/createSpec.js @@ -57,9 +57,18 @@ function createSpec(ext) { } implementations.forEach(implementation => { + if (fileWithoutExt === "import-css" && implementation !== nodeSass) { + // Skip CSS imports for all implementations that are not node-sass + // CSS imports is a legacy feature that we only support for node-sass + // See discussion https://github.com/webpack-contrib/sass-loader/pull/573/files?#r199109203 + return; + } + sassOptions.functions = customFunctions(implementation); + const name = implementation.info.split("\t")[0]; const css = implementation.renderSync(sassOptions).css; + fs.writeFileSync(path.join(basePath, "spec", name, fileWithoutExt + ".css"), css, "utf8"); }); }); diff --git a/test/tools/customFunctions.js b/test/tools/customFunctions.js index 57c9ea90..acb8b308 100644 --- a/test/tools/customFunctions.js +++ b/test/tools/customFunctions.js @@ -1,6 +1,6 @@ "use strict"; -module.exports = function(implementation) { +module.exports = function (implementation) { return { "headings($from: 0, $to: 6)"(from, to) { const f = from.getValue(); From 41b00792a1251539e12bb19185d7c1b5a7139511 Mon Sep 17 00:00:00 2001 From: Johannes Ewald Date: Wed, 1 Aug 2018 17:31:25 +0200 Subject: [PATCH 5/5] Only use job queue for node-sass See https://github.com/webpack-contrib/sass-loader/pull/573#discussion_r201725985 --- lib/loader.js | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/lib/loader.js b/lib/loader.js index 850c67c6..d3f30ec2 100644 --- a/lib/loader.js +++ b/lib/loader.js @@ -8,11 +8,7 @@ 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 = Number(process.env.UV_THREADPOOL_SIZE || 4); -let asyncSassJobQueue = null; +let nodeSassJobQueue = null; /** * The sass-loader makes node-sass available to webpack modules. @@ -41,21 +37,15 @@ function sassLoader(content) { addNormalizedDependency )); - if (asyncSassJobQueue === null) { - const implementation = options.implementation || require("node-sass"); - - validateSassVersion(implementation.info); - asyncSassJobQueue = async.queue(implementation.render, threadPoolSize - 1); - } - // Skip empty files, otherwise it will stop webpack, see issue #21 if (options.data.trim() === "") { callback(null, ""); 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); @@ -90,9 +80,11 @@ function sassLoader(content) { /** * Verifies that the implementation and version of Sass is supported by this loader. * - * @param {string} info + * @param {Object} module + * @returns {Function} */ -function validateSassVersion(info) { +function getRenderFuncFromSassImpl(module) { + const info = module.info; const components = info.split("\t"); if (components.length < 2) { @@ -110,13 +102,23 @@ function validateSassVersion(info) { 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."); } - } else { - throw new Error("Unknown Sass implementation \"" + implementation + "\"."); + // 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;