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

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

Merged
merged 20 commits into from
Apr 12, 2018
Merged
Show file tree
Hide file tree
Changes from 18 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
4 changes: 2 additions & 2 deletions .nycrc
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
"lib/**/*.js"
],
"lines": 97,
"statements": 91,
"statements": 97,
"functions": 100,
"branches": 89,
"branches": 91,
Copy link
Member

Choose a reason for hiding this comment

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

Can we return old values and add more tests?

Copy link
Member

Choose a reason for hiding this comment

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

/cc @Timer

Copy link
Contributor Author

@Timer Timer Apr 12, 2018

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@Timer oh sorry, all looks good 👍

"check-coverage": true
}
25 changes: 23 additions & 2 deletions lib/loader.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
"use strict";

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

Choose a reason for hiding this comment

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

Let move this variables before asyncSassJobQueue = async.queue(sass.render, threadPoolSize - 1);, it don't have sense in top of 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.

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

const asyncSassJobQueue = async.queue(sass.render, threadPoolSize - 1);
let asyncSassJobQueue = null;

/**
* The sass-loader makes node-sass available to webpack modules.
Expand All @@ -21,6 +20,28 @@ const asyncSassJobQueue = async.queue(sass.render, threadPoolSize - 1);
* @param {string} content
*/
function sassLoader(content) {
if (asyncSassJobQueue === null) {
let sass;
let sassVersion;

try {
sass = require("node-sass");
sassVersion = /^(\d+)/.exec(require("node-sass/package.json").version).pop();
} catch (e) {
throw new Error(
"`sass-loader` requires `node-sass` >=4. Please install a compatible version."
);
}

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 Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"eslint-plugin-jsdoc": "^2.4.0",
"file-loader": "^0.11.2",
"mocha": "^3.0.2",
"mock-require": "^3.0.1",
"node-sass": "^4.5.0",
"nyc": "^11.0.2",
"raw-loader": "^0.5.1",
Expand All @@ -53,7 +54,6 @@
"node": ">= 4.3 < 5.0.0 || >= 5.10"
},
"peerDependencies": {
"node-sass": "^4.0.0",

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

"webpack": "^2.0.0 || ^3.0.0 || ^4.0.0"
},
"keywords": [
Expand Down
36 changes: 36 additions & 0 deletions test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const customFunctions = require("./tools/customFunctions.js");
const pathToSassLoader = require.resolve("../lib/loader.js");
const testLoader = require("./tools/testLoader");
const sassLoader = require(pathToSassLoader);
const mockRequire = require("mock-require");

const CR = /\r/g;
const syntaxStyles = ["scss", "sass"];
Expand Down Expand Up @@ -256,6 +257,40 @@ describe("sass-loader", () => {
done();
});
});
it("should output a message when `node-sass` is missing", (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();

err.code = "MODULE_NOT_FOUND";
throw err;
};
runWebpack({
entry: pathToSassLoader + "!" + pathToErrorFile
}, (err) => {
module._resolveFilename = originalResolve;
mockRequire.reRequire("node-sass");
err.message.should.match(/Please install a compatible version/);
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();
});
});
});
});

Expand All @@ -265,6 +300,7 @@ function readCss(ext, id) {

function runWebpack(baseConfig, done) {
const webpackConfig = merge({
mode: "development",
output: {
path: path.join(__dirname, "output"),
filename: "bundle.js",
Expand Down