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

fix: respect webpack aliases (resolve.alias) #479

Merged
merged 3 commits into from
Apr 13, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
74 changes: 38 additions & 36 deletions lib/importsToResolve.js
Original file line number Diff line number Diff line change
@@ -1,58 +1,60 @@
"use strict";

const path = require("path");
const utils = require("loader-utils");

// libsass uses this precedence when importing files without extension
const extPrecedence = [".scss", ".sass", ".css"];
const matchModuleImport = /^~([^\/]+|@[^\/]+[\/][^\/]+)$/g;

/**
* When libsass tries to resolve an import, it uses a special algorithm.
* Since the sass-loader uses webpack to resolve the modules, we need to simulate that algorithm. This function
* returns an array of import paths to try.
* returns an array of import paths to try. The first entry in the array is always the original url
* to enable straight-forward webpack.config aliases.
*
* @param {string} request
* @param {string} url
* @returns {Array<string>}
*/
function importsToResolve(request) {
function importsToResolve(url) {
const request = utils.urlToRequest(url);
// Keep in mind: ext can also be something like '.datepicker' when the true extension is omitted and the filename contains a dot.
// @see https://github.com/webpack-contrib/sass-loader/issues/167
const ext = path.extname(request);

if (matchModuleImport.test(url)) {
return [url, request];
}

// libsass' import algorithm works like this:
// In case there is no file extension...
// - Prefer modules starting with '_'.
// - File extension precedence: .scss, .sass, .css.

// In case there is a file extension...
// - If the file is a CSS-file, do not include it all, but just link it via @import url().
// - The exact file name must match (no auto-resolving of '_'-modules).
if (ext === ".css") {
return [];
}
if (ext === ".scss" || ext === ".sass") {
return [url, request];
}

// Keep in mind: ext can also be something like '.datepicker' when the true extension is omitted and the filename contains a dot.
// @see https://github.com/webpack-contrib/sass-loader/issues/167
const ext = path.extname(request);
// In case there is no file extension...
// - Prefer modules starting with '_'.
// - File extension precedence: .scss, .sass, .css.
const basename = path.basename(request);
const dirname = path.dirname(request);
const startsWithUnderscore = basename.charAt(0) === "_";
const hasCssExt = ext === ".css";
const hasSassExt = ext === ".scss" || ext === ".sass";

// a module import is an identifier like 'bootstrap-sass'
// We also need to check for dirname since it might also be a deep import like 'bootstrap-sass/something'
let isModuleImport = request.charAt(0) !== "." && dirname === ".";

if (dirname.charAt(0) === "@") {
// Check whether it is a deep import from scoped npm package
// (i.e. @pkg/foo/file), if so, process import as file import;
// otherwise, if we import from root npm scoped package (i.e. @pkg/foo)
// process import as a module import.
isModuleImport = !(dirname.indexOf("/") > -1);

if (basename.charAt(0) === "_") {
return [
url,
`${ request }.scss`, `${ request }.sass`, `${ request }.css`
];
}

return (isModuleImport && [request]) || // Do not modify module imports
(hasCssExt && []) || // Do not import css files
(hasSassExt && [request]) || // Do not modify imports with explicit extensions
(startsWithUnderscore ? [] : extPrecedence) // Do not add underscore imports if there is already an underscore
.map(ext => "_" + basename + ext)
.concat(
extPrecedence.map(ext => basename + ext)
).map(
file => dirname + "/" + file // No path.sep required here, because imports inside SASS are usually with /
);
const dirname = path.dirname(request);

return [
url,
`${ dirname }/_${ basename }.scss`, `${ dirname }/_${ basename }.sass`, `${ dirname }/_${ basename }.css`,
`${ request }.scss`, `${ request }.sass`, `${ request }.css`
];
}

module.exports = importsToResolve;
3 changes: 1 addition & 2 deletions lib/webpackImporter.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
*/

const path = require("path");
const utils = require("loader-utils");
const tail = require("lodash.tail");
const importsToResolve = require("./importsToResolve");

Expand Down Expand Up @@ -63,7 +62,7 @@ function webpackImporter(resourcePath, resolve, addNormalizedDependency) {
return (url, prev, done) => {
startResolving(
dirContextFrom(prev),
importsToResolve(utils.urlToRequest(url))
importsToResolve(url)
) // Catch all resolving errors, return the original file and pass responsibility back to other custom importers
.catch(() => ({ file: url }))
.then(done);
Expand Down
25 changes: 17 additions & 8 deletions test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,14 @@ const loaderContextMock = {
};

Object.defineProperty(loaderContextMock, "options", {
set() {},
set() { },
get() {
throw new Error("webpack options are not allowed to be accessed anymore.");
}
});

syntaxStyles.forEach(ext => {
function execTest(testId, options) {
function execTest(testId, loaderOptions, webpackOptions) {
return new Promise((resolve, reject) => {
const baseConfig = merge({
entry: path.join(__dirname, ext, testId + "." + ext),
Expand All @@ -45,11 +45,11 @@ syntaxStyles.forEach(ext => {
test: new RegExp(`\\.${ ext }$`),
use: [
{ loader: "raw-loader" },
{ loader: pathToSassLoader, options }
{ loader: pathToSassLoader, options: loaderOptions }
]
}]
}
});
}, webpackOptions);

runWebpack(baseConfig, (err) => err ? reject(err) : resolve());
}).then(() => {
Expand Down Expand Up @@ -79,6 +79,13 @@ syntaxStyles.forEach(ext => {
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, "alias." + ext)
}
}
}));
});
describe("custom importers", () => {
it("should use custom importer", () => execTest("custom-importer", {
Expand Down Expand Up @@ -170,9 +177,11 @@ describe("sass-loader", () => {
test: /\.scss$/,
use: [
{ loader: testLoader.filename },
{ loader: pathToSassLoader, options: {
sourceMap: true
} }
{
loader: pathToSassLoader, options: {
sourceMap: true
}
}
]
}]
}
Expand All @@ -196,7 +205,7 @@ describe("sass-loader", () => {
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(8);
sourceMap.sources.should.have.length(9);
sourceMap.sources.forEach(sourcePath =>
fs.existsSync(path.resolve(sourceMap.sourceRoot, sourcePath))
);
Expand Down
3 changes: 3 additions & 0 deletions test/sass/alias.sass
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
a
color: red

1 change: 1 addition & 0 deletions test/sass/import-alias.sass
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@import path-to-alias
4 changes: 2 additions & 2 deletions test/sass/import-css.sass
Original file line number Diff line number Diff line change
@@ -1,7 +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 ../node_modules/css/some-css-module.css
@import ~css/some-css-module.css
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for spotting this. Seems like it slipped through with some of the last commits...

// 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 ../node_modules/css/some-css-module
@import ~css/some-css-module
2 changes: 2 additions & 0 deletions test/sass/imports.sass
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
@import another/module
/* @import another/underscore */
@import another/underscore
/* @import another/_underscore */
@import another/_underscore
/* @import ~sass/underscore */
@import ~sass/underscore
// Import a module with a dot in its name
Expand Down
3 changes: 3 additions & 0 deletions test/scss/alias.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
a {
color: red;
}
1 change: 1 addition & 0 deletions test/scss/import-alias.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
@import 'path-to-alias';
2 changes: 2 additions & 0 deletions test/scss/imports.scss
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
@import "another/module";
/* @import "another/underscore"; */
@import "another/underscore";
/* @import "another/underscore"; */
@import "another/_underscore";
/* @import "~scss/underscore"; */
@import "~scss/underscore";
// Import a module with a dot in its name
Expand Down
4 changes: 3 additions & 1 deletion test/tools/createSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ function createSpec(ext) {
const testNodeModules = path.relative(basePath, path.join(testFolder, "node_modules")) + path.sep;
const pathToBootstrap = path.relative(basePath, path.resolve(testFolder, "..", "node_modules", "bootstrap-sass"));
const pathToScopedNpmPkg = path.relative(basePath, path.resolve(testFolder, "node_modules", "@org", "pkg", "./index.scss"));
const pathToFooAlias = path.relative(basePath, path.resolve(testFolder, ext, "./alias." + ext));

fs.readdirSync(path.join(testFolder, ext))
.filter((file) => {
Expand All @@ -32,7 +33,8 @@ function createSpec(ext) {
url = url
.replace(/^~bootstrap-sass/, pathToBootstrap)
.replace(/^~@org\/pkg/, pathToScopedNpmPkg)
.replace(/^~/, testNodeModules);
.replace(/^~/, testNodeModules)
.replace(/^path-to-alias/, pathToFooAlias);
}
return {
file: url
Expand Down