Merge pull request #377 from jtangelder/fix/source-map

Fix incorrect source maps in certain cwds

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;

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;
         }
     }

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$/,

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,39 +151,58 @@ 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
                                 } }
                             ]
                         }]
                     }
                 }, 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");

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;