From 895044f39079efd107af895745c191a27a643033 Mon Sep 17 00:00:00 2001 From: Johannes Ewald Date: Thu, 16 Mar 2017 18:45:51 +0100 Subject: [PATCH] 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. --- src/index.js | 64 +++++++++++++++-------------------- src/removeSourceMappingUrl.js | 17 ++++++++++ test/helpers/createSpec.js | 10 ++++-- test/helpers/readFixture.js | 3 +- test/index.test.js | 9 ++--- 5 files changed, 57 insertions(+), 46 deletions(-) create mode 100644 src/removeSourceMappingUrl.js diff --git a/src/index.js b/src/index.js index fd901284..fdf11051 100644 --- a/src/index.js +++ b/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) { diff --git a/src/removeSourceMappingUrl.js b/src/removeSourceMappingUrl.js new file mode 100644 index 00000000..460e8f81 --- /dev/null +++ b/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; diff --git a/test/helpers/createSpec.js b/test/helpers/createSpec.js index 13856c5d..32bd3c98 100644 --- a/test/helpers/createSpec.js +++ b/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'); diff --git a/test/helpers/readFixture.js b/test/helpers/readFixture.js index 8503d743..bffc868a 100644 --- a/test/helpers/readFixture.js +++ b/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; diff --git a/test/index.test.js b/test/index.test.js index 974e9df7..35d16d50 100644 --- a/test/index.test.js +++ b/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 () => {