feat(source-maps): refactor source maps handling

* Remove sourceMappingURL from generated CSS since the less-loader does not know the final location of the source map
* Pass source map as object to the next loader

BREAKING CHANGE:

Since the map is now passed as an object to the next loader, this could potentially break if another loader than the css-loader is used. The css-loader accepts both.

Author: jhnns

src/index.js

@@ -1,24 +1,22 @@
 const less = require('less');
 const loaderUtils = require('loader-utils');
 const cloneDeep = require('clone-deep');
+const pify = require('pify');
+const removeSourceMappingUrl = require('./removeSourceMappingUrl');
 
 const trailingSlash = /[\\/]$/;
+const render = pify(less.render.bind(less));
 
 function lessLoader(source) {
   const loaderContext = this;
-  const options = Object.assign(
-    {
-      filename: this.resource,
-      paths: [],
-      plugins: [],
-      relativeUrls: true,
-      compress: Boolean(this.minimize),
-    },
-    cloneDeep(loaderUtils.getOptions(loaderContext)),
-  );
-  const cb = loaderContext.async();
-  const isSync = typeof cb !== 'function';
-  let finalCb = cb || loaderContext.callback;
+  const options = {
+    plugins: [],
+    relativeUrls: true,
+    compress: Boolean(this.minimize),
+    ...cloneDeep(loaderUtils.getOptions(loaderContext)),
+  };
+  const done = loaderContext.async();
+  const isSync = typeof done !== 'function';
   const webpackPlugin = {
     install(lessInstance, pluginManager) {
       const WebpackFileManager = getWebpackFileManager(loaderContext, options);
@@ -32,32 +30,26 @@ function lessLoader(source) {
     throw new Error('Synchronous compilation is not supported anymore. See https://github.com/webpack-contrib/less-loader/issues/84');
   }
 
+  // We need to set the filename because otherwise our WebpackFileManager will receive an undefined path for the entry
+  options.filename = loaderContext.resource;
+
   // It's safe to mutate the array now because it has already been cloned
   options.plugins.push(webpackPlugin);
 
-  if (options.sourceMap) {
-    options.sourceMap = {
-      outputSourceFiles: true,
-    };
-  }
-
-  less.render(source, options, (err, result) => {
-    const cb = finalCb;
-
-    // Less is giving us double callbacks sometimes :(
-    // Thus we need to mark the callback as "has been called"
-    if (!finalCb) {
-      return;
-    }
-    finalCb = null;
-
-    if (err) {
-      cb(formatLessRenderError(err));
-      return;
-    }
-
-    cb(null, result.css, result.map);
-  });
+  render(source, options)
+    .then(({ css, map }) => {
+      return {
+        // Removing the sourceMappingURL comment.
+        // See removeSourceMappingUrl.js for the reasoning behind this.
+        css: removeSourceMappingUrl(css),
+        map: typeof map === 'string' ? JSON.parse(map) : map,
+      };
+    }, (lessError) => {
+      throw formatLessRenderError(lessError);
+    })
+    .then(({ css, map }) => {
+      done(null, css, map);
+    }, done);
 }
 
 function getWebpackFileManager(loaderContext, query) {

src/removeSourceMappingUrl.js

@@ -0,0 +1,17 @@
+const matchSourceMappingUrl = /\/\*# sourceMappingURL=[^*]+\*\//;
+
+/**
+ * Removes the sourceMappingURL comment. This is necessary because the less-loader
+ * does not know where the final source map will be located. Thus, we remove every
+ * reference to source maps. In a regular setup, the css-loader will embed the
+ * source maps into the CommonJS module and the style-loader will translate it into
+ * base64 blob urls.
+ *
+ * @param {string} content
+ * @returns {string}
+ */
+function removeSourceMappingUrl(content) {
+  return content.replace(matchSourceMappingUrl, '');
+}
+
+module.exports = removeSourceMappingUrl;

test/helpers/createSpec.js

@@ -3,6 +3,7 @@
 const { exec } = require('child_process');
 const fs = require('fs');
 const path = require('path');
+const removeSourceMappingUrl = require('../../src/removeSourceMappingUrl');
 
 const projectPath = path.resolve(__dirname, '..', '..');
 const lessFixtures = path.resolve(__dirname, '..', 'fixtures', 'less');
@@ -61,8 +62,13 @@ testIds
           throw (err || new Error(stdout || stderr));
         }
 
-        const cssContent = fs.readFileSync(cssFile, 'utf8')
-          .replace(new RegExp(`(@import\\s+["'])${tildeReplacement}`, 'g'), '$1~');
+        // We remove the source mapping url because the less-loader will do it also.
+        // See removeSourceMappingUrl.js for the reasoning behind this.
+        const cssContent = removeSourceMappingUrl(
+          fs.readFileSync(cssFile, 'utf8')
+            // Change back tilde replacements
+            .replace(new RegExp(`(@import\\s+["'])${tildeReplacement}`, 'g'), '$1~'),
+        );
 
         fs.writeFileSync(lessFile, originalLessContent, 'utf8');
         fs.writeFileSync(cssFile, cssContent, 'utf8');

test/helpers/readFixture.js

@@ -22,8 +22,7 @@ function readSourceMap(testId) {
     .then(JSON.parse)
     // The map that is generated by our loader does not have a file property because the
     // output file is unknown when the Less API is used. That's why we need to remove that from our fixture.
-    .then(({ file, sourcesContent, ...map }) => map) // eslint-disable-line no-unused-vars
-    .then(JSON.stringify);
+    .then(({ file, ...map }) => map); // eslint-disable-line no-unused-vars
 }
 
 exports.readCssFixture = readCssFixture;

test/index.test.js

@@ -36,16 +36,13 @@ test('should transform urls', async () => {
 });
 
 test('should generate source maps', async () => {
-  const [{ inspect }, expectedSourceMap] = await Promise.all([
+  const [{ inspect }, expectedMap] = await Promise.all([
     compile('source-map', { sourceMap: true }),
     readSourceMap('source-map'),
   ]);
+  const [, actualMap] = inspect.arguments;
 
-  const map = JSON.parse(inspect.arguments[1]);
-
-  delete map.sourcesContent;
-
-  expect(JSON.stringify(map)).toEqual(expectedSourceMap);
+  expect(actualMap).toEqual(expectedMap);
 });
 
 test('should install plugins', async () => {