Skip to content

Commit

Permalink
fix: resolution logic when the includePaths option used (#827)
Browse files Browse the repository at this point in the history
  • Loading branch information
evilebottnawi committed Apr 12, 2020
1 parent 3ad5291 commit cbe5ad4
Show file tree
Hide file tree
Showing 6 changed files with 39 additions and 33 deletions.
14 changes: 6 additions & 8 deletions src/index.js
Expand Up @@ -33,20 +33,18 @@ function loader(content) {
: true;

if (shouldUseWebpackImporter) {
let modules = ['...'];

if (options.sassOptions && options.sassOptions.includePaths) {
modules = options.sassOptions.includePaths.concat(modules);
}

const resolve = this.getResolve({
modules,
mainFields: ['sass', 'style', 'main', '...'],
mainFiles: ['_index', 'index', '...'],
extensions: ['.scss', '.sass', '.css', '...'],
});

sassOptions.importer.push(webpackImporter(this, resolve));
const includePaths =
options.sassOptions && options.sassOptions.includePaths
? options.sassOptions.includePaths
: [];

sassOptions.importer.push(webpackImporter(this, resolve, includePaths));
}

const callback = this.async();
Expand Down
36 changes: 25 additions & 11 deletions src/webpackImporter.js
Expand Up @@ -27,23 +27,26 @@ const matchCss = /\.css$/i;
* (based on whether the call is sync or async) because otherwise node-sass doesn't exit.
*
*/
function webpackImporter(loaderContext, resolve) {
function webpackImporter(loaderContext, resolve, includePaths) {
function dirContextFrom(fileContext) {
return path.dirname(
// The first file is 'stdin' when we're using the data option
fileContext === 'stdin' ? loaderContext.resourcePath : fileContext
);
}

function startResolving(context, possibleRequests) {
if (possibleRequests.length === 0) {
return Promise.resolve();
function startResolving(resolutionMap) {
if (resolutionMap.length === 0) {
return Promise.reject();
}

const [{ context, possibleRequests }] = resolutionMap;

return resolve(context, possibleRequests[0])
.then((result) => {
// Add the resolvedFilename as dependency. Although we're also using stats.includedFiles, this might come
// in handy when an error occurs. In this case, we don't get stats.includedFiles from node-sass.
// Add the result as dependency.
// Although we're also using stats.includedFiles, this might come in handy when an error occurs.
// In this case, we don't get stats.includedFiles from node-sass/sass.
loaderContext.addDependency(path.normalize(result));

// By removing the CSS file extension, we trigger node-sass to include the CSS file instead of just linking it.
Expand All @@ -52,18 +55,29 @@ function webpackImporter(loaderContext, resolve) {
.catch(() => {
const [, ...tailResult] = possibleRequests;

return startResolving(context, tailResult);
if (tailResult.length === 0) {
const [, ...tailResolutionMap] = resolutionMap;

return startResolving(tailResolutionMap);
}

// eslint-disable-next-line no-param-reassign
resolutionMap[0].possibleRequests = tailResult;

return startResolving(resolutionMap);
});
}

return (url, prev, done) => {
const possibleRequests = getPossibleRequests(url);
const resolutionMap = []
.concat(includePaths)
.concat(dirContextFrom(prev))
.map((context) => ({ context, possibleRequests }));

startResolving(dirContextFrom(prev), possibleRequests)
startResolving(resolutionMap)
// Catch all resolving errors, return the original file and pass responsibility back to other custom importers
.catch(() => {
return { file: url };
})
.catch(() => ({ file: url }))
.then(done);
};
}
Expand Down
8 changes: 4 additions & 4 deletions test/__snapshots__/loader.test.js.snap
Expand Up @@ -462,9 +462,9 @@ exports[`loader should work and ignore all css "@import" at-rules (node-sass) (s
"@import url(~css/some-css-module.css);
@import url(./does/not/exist.css);
@import url(theme.css);
@import \\"http://fonts.googleapis.com/css?family=Droid+Sans\\";
@import http://fonts.googleapis.com/css?family=Droid+Sans;
@import url(theme);
@import \\"landscape\\" screen and (orientation: landscape);
@import landscape screen and (orientation: landscape);
.some-css-module {
background: hotpink; }
"
Expand All @@ -478,9 +478,9 @@ exports[`loader should work and ignore all css "@import" at-rules (node-sass) (s
"@import url(~css/some-css-module.css);
@import url(./does/not/exist.css);
@import url(theme.css);
@import \\"http://fonts.googleapis.com/css?family=Droid+Sans\\";
@import http://fonts.googleapis.com/css?family=Droid+Sans;
@import url(theme);
@import \\"landscape\\" screen and (orientation: landscape);
@import landscape screen and (orientation: landscape);
.some-css-module {
background: hotpink; }
"
Expand Down
8 changes: 0 additions & 8 deletions test/helpers/getCodeFromSass.js
Expand Up @@ -693,14 +693,6 @@ function getCodeFromSass(testId, options) {
.replace(/^~/, testNodeModules);
}

if (
isNodeSassImplementation &&
(url === 'landscape' || url.startsWith('http://'))
) {
// eslint-disable-next-line consistent-return
return;
}

return {
file: url,
};
Expand Down
4 changes: 3 additions & 1 deletion test/loader.test.js
Expand Up @@ -559,7 +559,9 @@ describe('loader', () => {
const options = {
sassOptions: {
includePaths: [
`node_modules/package-with-style-field-and-css/${syntax}`,
path.resolve(
`./test/node_modules/package-with-style-field-and-css/${syntax}`
),
],
},
implementation: getImplementationByName(implementationName),
Expand Down

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

0 comments on commit cbe5ad4

Please sign in to comment.