From e0a9b39ce8fd9d85da1cb0aba362cf4cc8b3e5d9 Mon Sep 17 00:00:00 2001 From: Johannes Ewald Date: Tue, 14 Feb 2017 16:01:41 +0100 Subject: [PATCH] Fix incorrect source maps in certain cwds All source map paths will be relative to process.cwd() from now on. This removes also the last dependency on this.options.context. node-sass source map options like sourceMapRoot, omitSourceMapUrl, sourceMapContents are now overridable. https://github.com/jtangelder/sass-loader/pull/374#issuecomment-279572238 --- lib/loader.js | 14 ++++--- lib/normalizeOptions.js | 25 ++++++++---- test/bootstrapSass/webpack.config.js | 2 +- test/index.test.js | 58 +++++++++++++++++++++------- test/tools/testLoader.js | 14 +++++++ 5 files changed, 86 insertions(+), 27 deletions(-) create mode 100644 test/tools/testLoader.js diff --git a/lib/loader.js b/lib/loader.js index 6574b0a7..3cfa45d3 100644 --- a/lib/loader.js +++ b/lib/loader.js @@ -54,14 +54,18 @@ function sassLoader(content) { if (result.map && result.map !== "{}") { result.map = JSON.parse(result.map); - result.map.file = resourcePath; - // The first source is 'stdin' according to libsass because we've used the data input - // Now let's override that value with the correct relative path - result.map.sources[0] = path.basename(resourcePath); - result.map.sourceRoot = path.dirname(resourcePath); + // result.map.file is an optional property that provides the output filename. + // Since we don't know the final filename in the webpack build chain yet, it makes no sense to have it. + delete result.map.file; + // The first source is 'stdin' according to node-sass because we've used the data input. + // Now let's override that value with the correct relative path. + // Since we specified options.sourceMap = path.join(process.cwd(), "/sass.map"); in normalizeOptions, + // we know that this path is relative to process.cwd(). This is how node-sass works. + result.map.sources[0] = path.relative(process.cwd(), resourcePath); // node-sass returns POSIX paths, that's why we need to transform them back to native paths. // This fixes an error on windows where the source-map module cannot resolve the source maps. // @see https://github.com/jtangelder/sass-loader/issues/366#issuecomment-279460722 + result.map.sourceRoot = path.normalize(result.map.sourceRoot); result.map.sources = result.map.sources.map(path.normalize); } else { result.map = null; diff --git a/lib/normalizeOptions.js b/lib/normalizeOptions.js index a2811933..103a0d9a 100644 --- a/lib/normalizeOptions.js +++ b/lib/normalizeOptions.js @@ -36,15 +36,24 @@ function normalizeOptions(loaderContext, content, webpackImporter) { // Not using the `this.sourceMap` flag because css source maps are different // @see https://github.com/webpack/css-loader/pull/40 if (options.sourceMap) { - // deliberately overriding the sourceMap option - // this value is (currently) ignored by libsass when using the data input instead of file input - // however, it is still necessary for correct relative paths in result.map.sources. - options.sourceMap = loaderContext.options.context + "/sass.map"; - options.omitSourceMapUrl = true; - - // If sourceMapContents option is not set, set it to true otherwise maps will be empty/null - // when exported by webpack-extract-text-plugin. + // Deliberately overriding the sourceMap option here. + // node-sass won't produce source maps if the data option is used and options.sourceMap is not a string. + // In case it is a string, options.sourceMap should be a path where the source map is written. + // But since we're using the data option, the source map will not actually be written, but + // all paths in sourceMap.sources will be relative to that path. + // Pretty complicated... :( + options.sourceMap = path.join(process.cwd(), "/sass.map"); + if ("sourceMapRoot" in options === false) { + options.sourceMapRoot = process.cwd(); + } + if ("omitSourceMapUrl" in options === false) { + // The source map url doesn't make sense because we don't know the output path + // The css-loader will handle that for us + options.omitSourceMapUrl = true; + } if ("sourceMapContents" in options === false) { + // If sourceMapContents option is not set, set it to true otherwise maps will be empty/null + // when exported by webpack-extract-text-plugin. options.sourceMapContents = true; } } diff --git a/test/bootstrapSass/webpack.config.js b/test/bootstrapSass/webpack.config.js index 4ef33fec..e30c04fd 100644 --- a/test/bootstrapSass/webpack.config.js +++ b/test/bootstrapSass/webpack.config.js @@ -9,7 +9,7 @@ module.exports = { path: path.resolve(__dirname, "../output"), filename: "bundle.bootstrap-sass.js" }, - devtool: "inline-source-map", + devtool: "source-map", module: { rules: [{ test: /\.scss$/, diff --git a/test/index.test.js b/test/index.test.js index 3471112d..a7c2e797 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -9,6 +9,7 @@ const merge = require("webpack-merge"); const customImporter = require("./tools/customImporter.js"); const customFunctions = require("./tools/customFunctions.js"); const pathToSassLoader = require.resolve("../lib/loader.js"); +const testLoader = require("./tools/testLoader"); const sassLoader = require(pathToSassLoader); const CR = /\r/g; @@ -17,6 +18,18 @@ const pathToErrorFileNotFound = path.resolve(__dirname, "./scss/error-file-not-f const pathToErrorFileNotFound2 = path.resolve(__dirname, "./scss/error-file-not-found-2.scss"); const pathToErrorFile = path.resolve(__dirname, "./scss/error.scss"); const pathToErrorImport = path.resolve(__dirname, "./scss/error-import.scss"); +const loaderContextMock = { + async: Function.prototype, + cacheable: Function.prototype, + dependency: Function.prototype +}; + +Object.defineProperty(loaderContextMock, "options", { + set() {}, + get() { + throw new Error("webpack options are not allowed to be accessed anymore."); + } +}); syntaxStyles.forEach(ext => { function execTest(testId, options) { @@ -138,23 +151,19 @@ describe("sass-loader", () => { ); }); describe("source maps", () => { - it("should compile without errors", () => - new Promise((resolve, reject) => { + function buildWithSourceMaps() { + return new Promise((resolve, reject) => { runWebpack({ entry: path.join(__dirname, "scss", "imports.scss"), - // We know that setting a custom context can confuse webpack when resolving source maps - context: path.join(__dirname, "scss"), output: { - filename: "bundle.source-maps.compile-without-errors.js" + filename: "bundle.source-maps.js" }, devtool: "source-map", module: { rules: [{ test: /\.scss$/, use: [ - { loader: "css-loader", options: { - sourceMap: true - } }, + { loader: testLoader.filename }, { loader: pathToSassLoader, options: { sourceMap: true } } @@ -162,15 +171,38 @@ describe("sass-loader", () => { }] } }, err => err ? reject(err) : resolve()); - }) - ); + }); + } + + 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; + }; + + 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(7); + sourceMap.sources.forEach(sourcePath => + fs.existsSync(path.resolve(sourceMap.sourceRoot, sourcePath)) + ); + + process.cwd = cwdGetter; + }); + }); }); describe("errors", () => { it("should throw an error in synchronous loader environments", () => { try { - sassLoader.call({ - async: Function.prototype - }, ""); + 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/jtangelder/sass-loader/issues/333"); diff --git a/test/tools/testLoader.js b/test/tools/testLoader.js new file mode 100644 index 00000000..71729f65 --- /dev/null +++ b/test/tools/testLoader.js @@ -0,0 +1,14 @@ +"use strict"; + +function testLoader(content, sourceMap) { + testLoader.content = content; + testLoader.sourceMap = sourceMap; + + return ""; +} + +testLoader.content = ""; +testLoader.sourceMap = null; +testLoader.filename = __filename; + +module.exports = testLoader;