From 16d97627a3c2104f6413e7378895f9526fd98137 Mon Sep 17 00:00:00 2001 From: Victor van Poppelen Date: Sun, 2 Aug 2020 00:46:35 -0700 Subject: [PATCH 1/8] refactor: extract LoaderContext from resolver used in Sass importer --- src/utils.js | 141 +++++++++++++++++++++++++++++++-------------------- 1 file changed, 86 insertions(+), 55 deletions(-) diff --git a/src/utils.js b/src/utils.js index 0464278d..920c16f9 100644 --- a/src/utils.js +++ b/src/utils.js @@ -211,22 +211,19 @@ const isModuleImport = /^~([^/]+|[^/]+\/|@[^/]+[/][^/]+|@[^/]+\/?|@[^/]+[/][^/]+ * * @param {string} url * @param {boolean} forWebpackResolver - * @param {Object} loaderContext + * @param {string} rootContext * @returns {Array} */ -export default function getPossibleRequests( - loaderContext, +function getPossibleRequests( // eslint-disable-next-line no-shadow url, - forWebpackResolver = false + forWebpackResolver = false, + rootContext = false ) { const request = urlToRequest( url, // Maybe it is server-relative URLs - forWebpackResolver && url.charAt(0) === '/' - ? loaderContext.rootContext - : // eslint-disable-next-line no-undefined - undefined + forWebpackResolver && rootContext ); // In case there is module request, send this to webpack resolver @@ -262,12 +259,26 @@ export default function getPossibleRequests( ]; } -const matchCss = /\.css$/i; +function promiseResolve(callbackResolve) { + return (context, request) => + new Promise((resolve, reject) => { + callbackResolve(context, request, (err, result) => { + if (err) reject(err); + else resolve(result); + }); + }); +} + const isSpecialModuleImport = /^~[^/]+$/; // `[drive_letter]:\` + `\\[server]\[sharename]\` const isNativeWin32Path = /^[a-zA-Z]:[/\\]|^\\\\/i; -function getWebpackImporter(loaderContext, implementation, includePaths) { +function getWebpackResolver( + implementation, + resolverFactory, + includePaths = [], + rootContext = false +) { async function startResolving(resolutionMap) { if (resolutionMap.length === 0) { return Promise.reject(); @@ -275,10 +286,8 @@ function getWebpackImporter(loaderContext, implementation, includePaths) { const [{ resolve, context, possibleRequests }] = resolutionMap; - let result; - try { - result = await resolve(context, possibleRequests[0]); + return await resolve(context, possibleRequests[0]); } catch (_ignoreError) { const [, ...tailResult] = possibleRequests; @@ -293,46 +302,42 @@ function getWebpackImporter(loaderContext, implementation, includePaths) { return startResolving(resolutionMap); } - - // 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. - return { file: result.replace(matchCss, '') }; } - const sassResolve = loaderContext.getResolve({ - alias: [], - aliasFields: [], - conditionNames: [], - descriptionFiles: [], - extensions: ['.sass', '.scss', '.css'], - exportsFields: [], - mainFields: [], - mainFiles: ['_index', 'index'], - modules: [], - restrictions: [/\.((sa|sc|c)ss)$/i], - }); - const webpackResolve = loaderContext.getResolve({ - conditionNames: ['sass', 'style'], - mainFields: ['sass', 'style', 'main', '...'], - mainFiles: ['_index', 'index', '...'], - extensions: ['.sass', '.scss', '.css'], - restrictions: [/\.((sa|sc|c)ss)$/i], - }); - - return (originalUrl, prev, done) => { - let request = originalUrl; + const sassResolve = promiseResolve( + resolverFactory({ + alias: [], + aliasFields: [], + conditionNames: [], + descriptionFiles: [], + extensions: ['.sass', '.scss', '.css'], + exportsFields: [], + mainFields: [], + mainFiles: ['_index', 'index'], + modules: [], + restrictions: [/\.((sa|sc|c)ss)$/i], + }) + ); + const webpackResolve = promiseResolve( + resolverFactory({ + conditionNames: ['sass', 'style'], + mainFields: ['sass', 'style', 'main', '...'], + mainFiles: ['_index', 'index', '...'], + extensions: ['.sass', '.scss', '.css'], + restrictions: [/\.((sa|sc|c)ss)$/i], + }) + ); - const isFileScheme = originalUrl.slice(0, 5).toLowerCase() === 'file:'; + return (context, request) => { + const originalRequest = request; + const isFileScheme = originalRequest.slice(0, 5).toLowerCase() === 'file:'; if (isFileScheme) { try { // eslint-disable-next-line no-param-reassign - request = url.fileURLToPath(originalUrl); + request = url.fileURLToPath(originalRequest); } catch (ignoreError) { + // eslint-disable-next-line no-param-reassign request = request.slice(7); } } @@ -347,8 +352,8 @@ function getWebpackImporter(loaderContext, implementation, includePaths) { // - Server-relative URLs - `/path/to/file.ext` (where `` is root context) // - Absolute path - `/full/path/to/file.ext` or `C:\\full\path\to\file.ext` !isFileScheme && - !originalUrl.startsWith('/') && - !isNativeWin32Path.test(originalUrl); + !originalRequest.startsWith('/') && + !isNativeWin32Path.test(originalRequest); if (includePaths.length > 0 && needEmulateSassResolver) { // The order of import precedence is as follows: @@ -360,19 +365,20 @@ function getWebpackImporter(loaderContext, implementation, includePaths) { // 5. Filesystem imports relative to a `SASS_PATH` path. // // Because `sass`/`node-sass` run custom importers before `3`, `4` and `5` points, we need to emulate this behavior to avoid wrong resolution. - const sassPossibleRequests = getPossibleRequests(loaderContext, request); + const sassPossibleRequests = getPossibleRequests(request); const isDartSass = implementation.info.includes('dart-sass'); // `node-sass` calls our importer before `1. Filesystem imports relative to the base file.`, so we need emulate this too if (!isDartSass) { resolutionMap = resolutionMap.concat({ resolve: sassResolve, - context: path.dirname(prev), + context: path.dirname(context), possibleRequests: sassPossibleRequests, }); } resolutionMap = resolutionMap.concat( + // eslint-disable-next-line no-shadow includePaths.map((context) => ({ resolve: sassResolve, context, @@ -382,21 +388,45 @@ function getWebpackImporter(loaderContext, implementation, includePaths) { } const webpackPossibleRequests = getPossibleRequests( - loaderContext, request, - true + true, + rootContext ); resolutionMap = resolutionMap.concat({ resolve: webpackResolve, - context: path.dirname(prev), + context: path.dirname(context), possibleRequests: webpackPossibleRequests, }); - startResolving(resolutionMap) + return startResolving(resolutionMap); + }; +} + +const matchCss = /\.css$/i; + +function getWebpackImporter(loaderContext, implementation, includePaths) { + const resolve = getWebpackResolver( + implementation, + loaderContext.getResolve, + includePaths, + loaderContext.rootContext + ); + + return (originalUrl, prev, done) => { + resolve(prev, originalUrl) + .then((result) => { + // 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. + done({ file: result.replace(matchCss, '') }); + }) // Catch all resolving errors, return the original file and pass responsibility back to other custom importers - .catch(() => ({ file: originalUrl })) - .then((result) => done(result)); + .catch(() => { + done({ file: originalUrl }); + }); }; } @@ -433,6 +463,7 @@ function getRenderFunctionFromSassImplementation(implementation) { export { getSassImplementation, getSassOptions, + getWebpackResolver, getWebpackImporter, getRenderFunctionFromSassImplementation, }; From 9fc94fadc2c8d4f80b9696b4ec8795c70fead1c5 Mon Sep 17 00:00:00 2001 From: Victor van Poppelen Date: Sun, 2 Aug 2020 02:01:53 -0700 Subject: [PATCH 2/8] feat: export a decoupled version of the Sass importer This will be used by `vue-jest` and potentially other projects, to write a Jest transform that can adequately mimic `sass-loader`'s behaviour. Closes #873 --- package.json | 1 + src/importer.js | 73 ++++++++++++++++++ test/__snapshots__/importer.test.js.snap | 95 ++++++++++++++++++++++++ test/importer.test.js | 22 ++++++ 4 files changed, 191 insertions(+) create mode 100644 src/importer.js create mode 100644 test/__snapshots__/importer.test.js.snap create mode 100644 test/importer.test.js diff --git a/package.json b/package.json index 7f193ef3..eb50b689 100644 --- a/package.json +++ b/package.json @@ -77,6 +77,7 @@ "css-loader": "^4.2.0", "del": "^5.1.0", "del-cli": "^3.0.1", + "enhanced-resolve": "^4.3.0", "eslint": "^7.6.0", "eslint-config-prettier": "^6.11.0", "eslint-plugin-import": "^2.21.2", diff --git a/src/importer.js b/src/importer.js new file mode 100644 index 00000000..5312d377 --- /dev/null +++ b/src/importer.js @@ -0,0 +1,73 @@ +import { getSassImplementation, getWebpackResolver } from './utils'; + +/** + * A factory function for creating a Sass importer that uses `sass-loader`'s + * resolution rules. + * + * @see https://sass-lang.com/documentation/js-api#importer + * + * This is useful when attempting to mimic `sass-loader`'s behaviour in contexts + * that do not support Webpack. For example, it could be used to write a Jest + * transform for testing files with Sass imports. + * + * The resulting Sass importer is asynchronous, so it can only be used with + * `sass.render()` and not `renderSync()`. + * + * Example usage: + * ```js + * import sass from 'sass'; + * import resolve from 'enhanced-resolve'; + * import createImporter from 'sass-loader/dist/importer'; + * import webpackConfig = './webpack.config'; + * + * const { resolve: { alias } } = webpackConfig; + * const resolverFactory = (options) => resolve.create({ alias, ...options }); + * const importer = createImporter(resolverFactory, sass); + * + * sass.render({ + * file: 'input.scss', + * importer, + * }, (err, result) => { + * // ... + * }); + * ``` + * + * @param {Function} resolverFactory - A factory function for creating a Webpack + * resolver. The resulting `resolve` function should be compatible with the + * asynchronous resolve function supplied by [`enhanced-resolve`]{@link + * https://github.com/webpack/enhanced-resolve}. In all likelihood you'll want + * to pass `resolve.create()` from `enhanced-resolve`, or a wrapped copy of + * it. + * @param {Object} [implementation] - The imported Sass implementation, both + * `sass` (Dart Sass) and `node-sass` are supported. If no implementation is + * supplied, `sass` will be preferred if it's available. + * @param {string[]} [includePaths] - The list of include paths passed to Sass. + * + * @returns {Function} + */ +export default function createSassImporter( + resolverFactory, + implementation = null, + includePaths = [] +) { + if (!implementation) { + // eslint-disable-next-line no-param-reassign + implementation = getSassImplementation(); + } + + const resolve = getWebpackResolver( + implementation, + resolverFactory, + includePaths + ); + + return (url, prev, done) => { + resolve(prev, url) + .then((result) => { + done({ file: result }); + }) + .catch(() => { + done(null); + }); + }; +} diff --git a/test/__snapshots__/importer.test.js.snap b/test/__snapshots__/importer.test.js.snap new file mode 100644 index 00000000..6bda69c9 --- /dev/null +++ b/test/__snapshots__/importer.test.js.snap @@ -0,0 +1,95 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`importer should resolve imports when passed to \`sass\` 1`] = ` +"@charset \\"UTF-8\\"; +/* @import another/module */ +@import url(http://example.com/something/from/the/interwebs); +.another-sass-module { + background: hotpink; +} + +/* @import another/underscore */ +.underscore { + background: hotpink; +} + +/* @import another/_underscore */ +.underscore { + background: hotpink; +} + +/* @import ~sass/underscore */ +.underscore-sass { + background: hotpink; +} + +/* @import ~sass/some.module */ +.some-sass-module { + background: hotpink; +} + +/* @import url(http://example.com/something/from/the/interwebs); */ +/* scoped import @import language */ +.scoped-import body { + font: 100% Helvetica, sans-serif; + color: #333; +} +.scoped-import nav ul { + margin: 0; + padding: 0; + list-style: none; +} +.scoped-import nav li { + display: inline-block; +} +.scoped-import nav a { + display: block; + padding: 6px 12px; + text-decoration: none; +} +.scoped-import .box { + -webkit-border-radius: 10px; + -moz-border-radius: 10px; + -ms-border-radius: 10px; + border-radius: 10px; +} +.scoped-import .message, .scoped-import .warning, .scoped-import .error, .scoped-import .success { + border: 1px solid #ccc; + padding: 10px; + color: #333; +} +.scoped-import .success { + border-color: green; +} +.scoped-import .error { + border-color: red; +} +.scoped-import .warning { + border-color: yellow; +} +.scoped-import .foo:before { + content: \\"\\"; +} +.scoped-import .bar:before { + content: \\"∑\\"; +} + +/* @import util */ +.util { + color: hotpink; +} + +/* @import ~module */ +.module { + background: hotpink; +} + +/* @import ~another */ +.another-scss-module-from-node-modules { + background: hotpink; +} + +a { + color: red; +}" +`; diff --git a/test/importer.test.js b/test/importer.test.js new file mode 100644 index 00000000..023b4295 --- /dev/null +++ b/test/importer.test.js @@ -0,0 +1,22 @@ +import sass from 'sass'; +import resolve from 'enhanced-resolve'; + +import createSassImporter from '../src/importer'; + +describe('importer', () => { + it('should resolve imports when passed to `sass`', (done) => { + const importer = createSassImporter(resolve.create); + + sass.render( + { + file: 'test/sass/imports.sass', + importer, + }, + (err, result) => { + expect(result.css.toString()).toMatchSnapshot(); + + done(err); + } + ); + }); +}); From b0f9b9a9b9e2806ab1bec9e8dd55ca9340e09fca Mon Sep 17 00:00:00 2001 From: Victor van Poppelen Date: Sun, 2 Aug 2020 03:01:35 -0700 Subject: [PATCH 3/8] test: add coverage for importer --- test/importer.test.js | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/importer.test.js b/test/importer.test.js index 023b4295..65a13bb8 100644 --- a/test/importer.test.js +++ b/test/importer.test.js @@ -19,4 +19,26 @@ describe('importer', () => { } ); }); + + it('should pass `null` to `done()` when resolution fails', (done) => { + let spy; + // eslint-disable-next-line no-shadow + const importer = (url, prev, done) => { + spy = jest.fn(done); + createSassImporter(resolve.create, sass)(url, prev, spy); + }; + + sass.render( + { + file: 'test/sass/error-file-not-found.sass', + importer, + }, + (err) => { + expect(spy).toHaveBeenCalledWith(null); + expect(err.toString()).toMatch("Can't find stylesheet to import."); + + done(); + } + ); + }); }); From c649db0343560cdf5912779242a2246a79b52c36 Mon Sep 17 00:00:00 2001 From: Victor van Poppelen Date: Tue, 4 Aug 2020 23:35:12 -0700 Subject: [PATCH 4/8] fix: remove exported importer function As discussed in code review, this is more the responsibility of the consumer, as `sass-loader/dist/utils.js` already exports all the necessary functionality. --- package.json | 1 - src/importer.js | 73 ------------------ src/utils.js | 21 ++++++ test/__snapshots__/importer.test.js.snap | 95 ------------------------ test/importer.test.js | 44 ----------- 5 files changed, 21 insertions(+), 213 deletions(-) delete mode 100644 src/importer.js delete mode 100644 test/__snapshots__/importer.test.js.snap delete mode 100644 test/importer.test.js diff --git a/package.json b/package.json index eb50b689..7f193ef3 100644 --- a/package.json +++ b/package.json @@ -77,7 +77,6 @@ "css-loader": "^4.2.0", "del": "^5.1.0", "del-cli": "^3.0.1", - "enhanced-resolve": "^4.3.0", "eslint": "^7.6.0", "eslint-config-prettier": "^6.11.0", "eslint-plugin-import": "^2.21.2", diff --git a/src/importer.js b/src/importer.js deleted file mode 100644 index 5312d377..00000000 --- a/src/importer.js +++ /dev/null @@ -1,73 +0,0 @@ -import { getSassImplementation, getWebpackResolver } from './utils'; - -/** - * A factory function for creating a Sass importer that uses `sass-loader`'s - * resolution rules. - * - * @see https://sass-lang.com/documentation/js-api#importer - * - * This is useful when attempting to mimic `sass-loader`'s behaviour in contexts - * that do not support Webpack. For example, it could be used to write a Jest - * transform for testing files with Sass imports. - * - * The resulting Sass importer is asynchronous, so it can only be used with - * `sass.render()` and not `renderSync()`. - * - * Example usage: - * ```js - * import sass from 'sass'; - * import resolve from 'enhanced-resolve'; - * import createImporter from 'sass-loader/dist/importer'; - * import webpackConfig = './webpack.config'; - * - * const { resolve: { alias } } = webpackConfig; - * const resolverFactory = (options) => resolve.create({ alias, ...options }); - * const importer = createImporter(resolverFactory, sass); - * - * sass.render({ - * file: 'input.scss', - * importer, - * }, (err, result) => { - * // ... - * }); - * ``` - * - * @param {Function} resolverFactory - A factory function for creating a Webpack - * resolver. The resulting `resolve` function should be compatible with the - * asynchronous resolve function supplied by [`enhanced-resolve`]{@link - * https://github.com/webpack/enhanced-resolve}. In all likelihood you'll want - * to pass `resolve.create()` from `enhanced-resolve`, or a wrapped copy of - * it. - * @param {Object} [implementation] - The imported Sass implementation, both - * `sass` (Dart Sass) and `node-sass` are supported. If no implementation is - * supplied, `sass` will be preferred if it's available. - * @param {string[]} [includePaths] - The list of include paths passed to Sass. - * - * @returns {Function} - */ -export default function createSassImporter( - resolverFactory, - implementation = null, - includePaths = [] -) { - if (!implementation) { - // eslint-disable-next-line no-param-reassign - implementation = getSassImplementation(); - } - - const resolve = getWebpackResolver( - implementation, - resolverFactory, - includePaths - ); - - return (url, prev, done) => { - resolve(prev, url) - .then((result) => { - done({ file: result }); - }) - .catch(() => { - done(null); - }); - }; -} diff --git a/src/utils.js b/src/utils.js index 920c16f9..3ec2dbda 100644 --- a/src/utils.js +++ b/src/utils.js @@ -24,6 +24,11 @@ function getDefaultSassImplementation() { return require(sassImplPkg); } +/** + * @public + * This function is not Webpack-specific and can be used by tools wishing to + * mimic `sass-loader`'s behaviour, so its signature should not be changed. + */ function getSassImplementation(implementation) { let resolvedImplementation = implementation; @@ -273,6 +278,22 @@ const isSpecialModuleImport = /^~[^/]+$/; // `[drive_letter]:\` + `\\[server]\[sharename]\` const isNativeWin32Path = /^[a-zA-Z]:[/\\]|^\\\\/i; +/** + * @public + * Create the resolve function used in the custom Sass importer. + * + * Can be used by external tools to mimic how `sass-loader` works, for example + * in a Jest transform. Such usages will want to wrap `resolve.create` from + * [`enhanced-resolve`]{@link https://github.com/webpack/enhanced-resolve} to + * pass as the `resolverFactory` argument. + * + * @param {Object} implementation - The imported Sass implementation, both + * `sass` (Dart Sass) and `node-sass` are supported. + * @param {Function} resolverFactory - A factory function for creating a Webpack + * resolver. + * @param {string[]} [includePaths] - The list of include paths passed to Sass. + * @param {boolean} [rootContext] - The configured Webpack root context. + */ function getWebpackResolver( implementation, resolverFactory, diff --git a/test/__snapshots__/importer.test.js.snap b/test/__snapshots__/importer.test.js.snap deleted file mode 100644 index 6bda69c9..00000000 --- a/test/__snapshots__/importer.test.js.snap +++ /dev/null @@ -1,95 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`importer should resolve imports when passed to \`sass\` 1`] = ` -"@charset \\"UTF-8\\"; -/* @import another/module */ -@import url(http://example.com/something/from/the/interwebs); -.another-sass-module { - background: hotpink; -} - -/* @import another/underscore */ -.underscore { - background: hotpink; -} - -/* @import another/_underscore */ -.underscore { - background: hotpink; -} - -/* @import ~sass/underscore */ -.underscore-sass { - background: hotpink; -} - -/* @import ~sass/some.module */ -.some-sass-module { - background: hotpink; -} - -/* @import url(http://example.com/something/from/the/interwebs); */ -/* scoped import @import language */ -.scoped-import body { - font: 100% Helvetica, sans-serif; - color: #333; -} -.scoped-import nav ul { - margin: 0; - padding: 0; - list-style: none; -} -.scoped-import nav li { - display: inline-block; -} -.scoped-import nav a { - display: block; - padding: 6px 12px; - text-decoration: none; -} -.scoped-import .box { - -webkit-border-radius: 10px; - -moz-border-radius: 10px; - -ms-border-radius: 10px; - border-radius: 10px; -} -.scoped-import .message, .scoped-import .warning, .scoped-import .error, .scoped-import .success { - border: 1px solid #ccc; - padding: 10px; - color: #333; -} -.scoped-import .success { - border-color: green; -} -.scoped-import .error { - border-color: red; -} -.scoped-import .warning { - border-color: yellow; -} -.scoped-import .foo:before { - content: \\"\\"; -} -.scoped-import .bar:before { - content: \\"∑\\"; -} - -/* @import util */ -.util { - color: hotpink; -} - -/* @import ~module */ -.module { - background: hotpink; -} - -/* @import ~another */ -.another-scss-module-from-node-modules { - background: hotpink; -} - -a { - color: red; -}" -`; diff --git a/test/importer.test.js b/test/importer.test.js deleted file mode 100644 index 65a13bb8..00000000 --- a/test/importer.test.js +++ /dev/null @@ -1,44 +0,0 @@ -import sass from 'sass'; -import resolve from 'enhanced-resolve'; - -import createSassImporter from '../src/importer'; - -describe('importer', () => { - it('should resolve imports when passed to `sass`', (done) => { - const importer = createSassImporter(resolve.create); - - sass.render( - { - file: 'test/sass/imports.sass', - importer, - }, - (err, result) => { - expect(result.css.toString()).toMatchSnapshot(); - - done(err); - } - ); - }); - - it('should pass `null` to `done()` when resolution fails', (done) => { - let spy; - // eslint-disable-next-line no-shadow - const importer = (url, prev, done) => { - spy = jest.fn(done); - createSassImporter(resolve.create, sass)(url, prev, spy); - }; - - sass.render( - { - file: 'test/sass/error-file-not-found.sass', - importer, - }, - (err) => { - expect(spy).toHaveBeenCalledWith(null); - expect(err.toString()).toMatch("Can't find stylesheet to import."); - - done(); - } - ); - }); -}); From 0d59f66e665a6dc8ca04ef12d4e2e01d345fad7c Mon Sep 17 00:00:00 2001 From: Victor van Poppelen Date: Sat, 15 Aug 2020 15:57:40 -0700 Subject: [PATCH 5/8] refactor: remove public annotation from getSassImplementation We can reduce our API surface slightly by not considering `getSassImplementation` as a public function, and instead using to determine defaults from within the only public function in `utils.js`: `getWebpackResolver()`. --- src/utils.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/utils.js b/src/utils.js index 3ec2dbda..6dd783e4 100644 --- a/src/utils.js +++ b/src/utils.js @@ -24,11 +24,6 @@ function getDefaultSassImplementation() { return require(sassImplPkg); } -/** - * @public - * This function is not Webpack-specific and can be used by tools wishing to - * mimic `sass-loader`'s behaviour, so its signature should not be changed. - */ function getSassImplementation(implementation) { let resolvedImplementation = implementation; @@ -287,16 +282,19 @@ const isNativeWin32Path = /^[a-zA-Z]:[/\\]|^\\\\/i; * [`enhanced-resolve`]{@link https://github.com/webpack/enhanced-resolve} to * pass as the `resolverFactory` argument. * - * @param {Object} implementation - The imported Sass implementation, both - * `sass` (Dart Sass) and `node-sass` are supported. * @param {Function} resolverFactory - A factory function for creating a Webpack * resolver. + * @param {Object} [implementation] - The imported Sass implementation, both + * `sass` (Dart Sass) and `node-sass` are supported. Defaults to `sass` if it + * is available. * @param {string[]} [includePaths] - The list of include paths passed to Sass. * @param {boolean} [rootContext] - The configured Webpack root context. + * + * @throws If a compatible Sass implementation cannot be found. */ function getWebpackResolver( - implementation, resolverFactory, + implementation, includePaths = [], rootContext = false ) { @@ -325,6 +323,9 @@ function getWebpackResolver( } } + const isDartSass = getSassImplementation(implementation).info.includes( + 'dart-sass' + ); const sassResolve = promiseResolve( resolverFactory({ alias: [], @@ -387,7 +388,6 @@ function getWebpackResolver( // // Because `sass`/`node-sass` run custom importers before `3`, `4` and `5` points, we need to emulate this behavior to avoid wrong resolution. const sassPossibleRequests = getPossibleRequests(request); - const isDartSass = implementation.info.includes('dart-sass'); // `node-sass` calls our importer before `1. Filesystem imports relative to the base file.`, so we need emulate this too if (!isDartSass) { @@ -428,8 +428,8 @@ const matchCss = /\.css$/i; function getWebpackImporter(loaderContext, implementation, includePaths) { const resolve = getWebpackResolver( - implementation, loaderContext.getResolve, + implementation, includePaths, loaderContext.rootContext ); From 18106c3a17e1f5f93a55d97f3b4dc1cf6862aa34 Mon Sep 17 00:00:00 2001 From: Victor van Poppelen Date: Sat, 15 Aug 2020 18:15:21 -0700 Subject: [PATCH 6/8] test: add test suite for getWebpackResolver Because `getWebpackResolver` is now part of `utils.js`' public API, we should test it separately so that we have some protection against its API changing drastically from potential future refactors. --- package.json | 1 + test/resolver.test.js | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+) create mode 100644 test/resolver.test.js diff --git a/package.json b/package.json index 7f193ef3..eb50b689 100644 --- a/package.json +++ b/package.json @@ -77,6 +77,7 @@ "css-loader": "^4.2.0", "del": "^5.1.0", "del-cli": "^3.0.1", + "enhanced-resolve": "^4.3.0", "eslint": "^7.6.0", "eslint-config-prettier": "^6.11.0", "eslint-plugin-import": "^2.21.2", diff --git a/test/resolver.test.js b/test/resolver.test.js new file mode 100644 index 00000000..d5982a26 --- /dev/null +++ b/test/resolver.test.js @@ -0,0 +1,36 @@ +import { fileURLToPath } from 'url'; + +import enhanced from 'enhanced-resolve'; + +import { getWebpackResolver } from '../src/utils'; + +/** + * Because `getWebpackResolver` is a public function that can be imported by + * external packages, we want to test it separately to ensure its API does not + * change unexpectedly. + */ +describe('getWebpackResolver', () => { + const resolve = (request, ...options) => + getWebpackResolver(enhanced.create, ...options)(__filename, request); + + it('should resolve .scss from node_modules', async () => { + expect(await resolve('scss/style')).toMatch(/style\.scss$/); + }); + + it('should resolve from passed `includePaths`', async () => { + expect(await resolve('empty', null, [`${__dirname}/scss`])).toMatch( + /empty\.scss$/ + ); + }); + + it('should reject when file cannot be resolved', async () => { + await expect(resolve('foo/bar/baz')).rejects.toBe(); + }); + + it('should strip an invalid file URL of its scheme', async () => { + const invalidFileURL = 'file://scss/empty'; + + expect(() => fileURLToPath(invalidFileURL)).toThrow(); + expect(await resolve(invalidFileURL)).toMatch(/empty\.scss$/); + }); +}); From c2dc54d80b4ca41c162fefc26dba57272245bd36 Mon Sep 17 00:00:00 2001 From: Victor van Poppelen Date: Sat, 15 Aug 2020 18:40:47 -0700 Subject: [PATCH 7/8] test: fix platform-dependent resolver test --- test/resolver.test.js | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/test/resolver.test.js b/test/resolver.test.js index d5982a26..b3e7b352 100644 --- a/test/resolver.test.js +++ b/test/resolver.test.js @@ -27,10 +27,17 @@ describe('getWebpackResolver', () => { await expect(resolve('foo/bar/baz')).rejects.toBe(); }); - it('should strip an invalid file URL of its scheme', async () => { - const invalidFileURL = 'file://scss/empty'; - - expect(() => fileURLToPath(invalidFileURL)).toThrow(); - expect(await resolve(invalidFileURL)).toMatch(/empty\.scss$/); - }); + if (process.platform !== 'win32') { + // a `file:` URI with two `/`s indicates the next segment is a hostname, + // which Node restricts to `localhost` on Unix platforms. Because it is + // nevertheless commonly used, the resolver converts it to a relative path. + // Node does allow specifying remote hosts in the Windows environment, so + // this test is restricted to Unix platforms. + it('should convert an invalid file URL with an erroneous hostname to a relative path', async () => { + const invalidFileURL = 'file://scss/empty'; + + expect(() => fileURLToPath(invalidFileURL)).toThrow(); + expect(await resolve(invalidFileURL)).toMatch(/empty\.scss$/); + }); + } }); From 6a93f159709aa62f1981484f5855246894428b18 Mon Sep 17 00:00:00 2001 From: Victor van Poppelen Date: Sat, 15 Aug 2020 21:57:37 -0700 Subject: [PATCH 8/8] fix: revert embedding getSassImplementation in getWebpackResolver Using `getSassImplementation()` inside the resolver factory is not a good idea not only because it means it will be called twice for normal usage of the loader, but also because consumers of `getWebpackResolver` are almost certainly also calling Sass itself, meaning they'll need to use `getSassImplementation()` anyway to make sure they're using the same implementation as the resolver thinks is being used. --- src/utils.js | 14 ++++++++------ test/resolver.test.js | 5 +++-- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/utils.js b/src/utils.js index 6dd783e4..fa3c0112 100644 --- a/src/utils.js +++ b/src/utils.js @@ -24,6 +24,11 @@ function getDefaultSassImplementation() { return require(sassImplPkg); } +/** + * @public + * This function is not Webpack-specific and can be used by tools wishing to + * mimic `sass-loader`'s behaviour, so its signature should not be changed. + */ function getSassImplementation(implementation) { let resolvedImplementation = implementation; @@ -284,9 +289,8 @@ const isNativeWin32Path = /^[a-zA-Z]:[/\\]|^\\\\/i; * * @param {Function} resolverFactory - A factory function for creating a Webpack * resolver. - * @param {Object} [implementation] - The imported Sass implementation, both - * `sass` (Dart Sass) and `node-sass` are supported. Defaults to `sass` if it - * is available. + * @param {Object} implementation - The imported Sass implementation, both + * `sass` (Dart Sass) and `node-sass` are supported. * @param {string[]} [includePaths] - The list of include paths passed to Sass. * @param {boolean} [rootContext] - The configured Webpack root context. * @@ -323,9 +327,7 @@ function getWebpackResolver( } } - const isDartSass = getSassImplementation(implementation).info.includes( - 'dart-sass' - ); + const isDartSass = implementation.info.includes('dart-sass'); const sassResolve = promiseResolve( resolverFactory({ alias: [], diff --git a/test/resolver.test.js b/test/resolver.test.js index b3e7b352..f45a0f7d 100644 --- a/test/resolver.test.js +++ b/test/resolver.test.js @@ -1,6 +1,7 @@ import { fileURLToPath } from 'url'; import enhanced from 'enhanced-resolve'; +import sass from 'sass'; import { getWebpackResolver } from '../src/utils'; @@ -11,14 +12,14 @@ import { getWebpackResolver } from '../src/utils'; */ describe('getWebpackResolver', () => { const resolve = (request, ...options) => - getWebpackResolver(enhanced.create, ...options)(__filename, request); + getWebpackResolver(enhanced.create, sass, ...options)(__filename, request); it('should resolve .scss from node_modules', async () => { expect(await resolve('scss/style')).toMatch(/style\.scss$/); }); it('should resolve from passed `includePaths`', async () => { - expect(await resolve('empty', null, [`${__dirname}/scss`])).toMatch( + expect(await resolve('empty', [`${__dirname}/scss`])).toMatch( /empty\.scss$/ ); });