From ef11ce1d5dd4c95ec3c31c658eee91e67caf04be Mon Sep 17 00:00:00 2001 From: Nick Petruzzelli Date: Fri, 3 Aug 2018 18:06:34 -0400 Subject: [PATCH 1/6] Create predicatable source maps by providing `resourcePath` as `options.file` and `options.outFile` (outFile does not cause anything to be be written to the file system.) Signed-off-by: Nick Petruzzelli --- lib/loader.js | 8 ------- lib/normalizeOptions.js | 46 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/lib/loader.js b/lib/loader.js index d3f30ec2..e0c2169b 100644 --- a/lib/loader.js +++ b/lib/loader.js @@ -55,14 +55,6 @@ function sassLoader(content) { if (result.map && result.map !== "{}") { result.map = JSON.parse(result.map); - // 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/webpack-contrib/sass-loader/issues/366#issuecomment-279460722 diff --git a/lib/normalizeOptions.js b/lib/normalizeOptions.js index ad1ca1e2..739db7db 100644 --- a/lib/normalizeOptions.js +++ b/lib/normalizeOptions.js @@ -32,6 +32,52 @@ 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) { + /** + * > Path to a file for LibSass to compile. + * @see node-sass [file]{@link https://github.com/sass/node-sass#file} + * + * > **Special behaviours** + * > + * > In the case that both file and data options are set, node-sass will + * > give precedence to data and use file to calculate paths in + * > sourcemaps. + * @see node-sass [Special behaviours]{@link https://github.com/sass/node-sass#special-behaviours} + * + * Another benefit to setting this is that `stdin`/`stdout` will no + * longer appear in `map.sources`. There will be no need to update + * `map.sources`, `map.names`, or similar. + * + * @type {String} [options.file=null] + */ + options.file = resourcePath; + + /** + * > **Special:** Required when `sourceMap` is a truthy value + * > + * > Specify the intended location of the output file. Strongly + * > recommended when outputting source maps so that they can properly + * > refer back to their intended files. + * > + * > **Attention** enabling this option will not write the file on disk + * > for you, it's for internal reference purpose only (to generate the + * > map for example). + * @see node-sass [outFile]{@link https://github.com/sass/node-sass#outfile} + * + * Even though we are always setting `sourceMap` to a string, the + * documentation says this is required, so set it to the expected value + * to comply with the requirement. + * + * `sass-loader` isn't responsible for writing the map, so it doesn't + * have to worry about updating the map with a transformation that + * changes locations (suchs as map.file or map.sources). + * + * Changing the file extension counts as changing the location because + * it changes the path. + * + * @type {String | null} [options.outFile=null] + */ + options.outFile = resourcePath; + // 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. From 3c928a40d02eded4ddbe9ff0092f63c1d9dd0fd6 Mon Sep 17 00:00:00 2001 From: Nick Petruzzelli Date: Fri, 3 Aug 2018 18:20:57 -0400 Subject: [PATCH 2/6] Allow the user to opt out of legacy handling, but keep it as the default out of sensitivity to existing scripts that may expect this behavior. Signed-off-by: Nick Petruzzelli --- README.md | 31 +++++++++++++++++++++++ lib/loader.js | 16 ++++++++---- lib/normalizeOptions.js | 55 +++++++++++++++++++++++++++-------------- 3 files changed, 78 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index bbc13b48..7634f387 100644 --- a/README.md +++ b/README.md @@ -227,6 +227,37 @@ module.exports = { If you want to edit the original Sass files inside Chrome, [there's a good blog post](https://medium.com/@toolmantim/getting-started-with-css-sourcemaps-and-in-browser-sass-editing-b4daab987fb0). Checkout [test/sourceMap](https://github.com/webpack-contrib/sass-loader/tree/master/test) for a running example. +#### Opting out of legacy source maps behavior + +To receive source maps as URLs relative to `loader.resourcePath`, set the `legacySourceMaps` option to `false`. + +```javascript +module.exports = { + // ... + devtool: "source-map", + module: { + rules: [{ + test: /\.scss$/, + use: [{ + loader: "style-loader" + }, { + loader: "css-loader", options: { + sourceMap: true + } + }, { + loader: "sass-loader", options: { + legacySourceMaps: false, + sourceMap: true + } + }] + }] + } +}; +``` + +_\* If you are using a plugin to extract source maps, such as `webpack.SourceMapDevToolPlugin`, make sure to set `devtool: false`._ + + ### Environment variables If you want to prepend Sass code before the actual entry file, you can set the `data` option. In this case, the sass-loader will not override the `data` option but just append the entry's content. This is especially useful when some of your Sass variables depend on the environment: diff --git a/lib/loader.js b/lib/loader.js index e0c2169b..48e90eb5 100644 --- a/lib/loader.js +++ b/lib/loader.js @@ -55,11 +55,17 @@ function sassLoader(content) { if (result.map && result.map !== "{}") { result.map = JSON.parse(result.map); - // 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/webpack-contrib/sass-loader/issues/366#issuecomment-279460722 - result.map.sourceRoot = path.normalize(result.map.sourceRoot); - result.map.sources = result.map.sources.map(path.normalize); + if (options.legacySourceMaps !== false) { + // @see https://github.com/webpack-contrib/sass-loader/issues/366#issuecomment-279460722 + result.map.sourceRoot = path.normalize(result.map.sourceRoot); + result.map.sources = result.map.sources.map(path.normalize); + } + // else { + // // node-sass returns relative URL paths, not file system paths + // // (POSIX or otherwise.) Aside from possible differences in the + // // path separator, relative URL paths are (almost always) valid + // // file system paths, allowing them to be used as such. + // } } else { result.map = null; } diff --git a/lib/normalizeOptions.js b/lib/normalizeOptions.js index 739db7db..f8c864ac 100644 --- a/lib/normalizeOptions.js +++ b/lib/normalizeOptions.js @@ -78,25 +78,42 @@ function normalizeOptions(loaderContext, content, webpackImporter) { */ options.outFile = resourcePath; - // 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; + if (option.legacySourceMaps !== false) { + // 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; + } + } else { + /** + * > **Special: ** Setting the `sourceMap` option requires also setting + * > the `outFile` option + * > + * > Enables the outputting of a source map during `render` and + * > `renderSync`. When `sourceMap === true`, the value of `outFile` is + * > used as the target output location for the source map. When + * > `typeof sourceMap === "string"`, the value of `sourceMap` will be + * > used as the writing location for the file. + * @see node-sass [sourceMap]{@link https://github.com/sass/node-sass#sourcemap} + * + * @type {Boolean | String | undefined} [options.sourceMap=undefined] + */ + options.sourceMap = options.outFile + ".map"; } } From d3733a38c5719d59c28446c33365dbf95a4d9cab Mon Sep 17 00:00:00 2001 From: Nick Petruzzelli Date: Fri, 3 Aug 2018 20:08:20 -0400 Subject: [PATCH 3/6] Fix `option not defined` reference error. Signed-off-by: Nick Petruzzelli --- lib/normalizeOptions.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/normalizeOptions.js b/lib/normalizeOptions.js index f8c864ac..ce9071ba 100644 --- a/lib/normalizeOptions.js +++ b/lib/normalizeOptions.js @@ -78,7 +78,7 @@ function normalizeOptions(loaderContext, content, webpackImporter) { */ options.outFile = resourcePath; - if (option.legacySourceMaps !== false) { + if (options.legacySourceMaps !== false) { // 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. From a7122d0d5dff95258584613a947301648595d004 Mon Sep 17 00:00:00 2001 From: Nick Petruzzelli Date: Fri, 3 Aug 2018 20:50:25 -0400 Subject: [PATCH 4/6] Remove build error caused by the presence of the file property on the map. Signed-off-by: Nick Petruzzelli --- lib/loader.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/loader.js b/lib/loader.js index 48e90eb5..e519b03c 100644 --- a/lib/loader.js +++ b/lib/loader.js @@ -59,6 +59,11 @@ function sassLoader(content) { // @see https://github.com/webpack-contrib/sass-loader/issues/366#issuecomment-279460722 result.map.sourceRoot = path.normalize(result.map.sourceRoot); result.map.sources = result.map.sources.map(path.normalize); + + // Legacy test considers the presence of the `file` attribute to + // be an invalid source map. Until the tests are updated, + // continue with removing the property. + delete result.map.file; } // else { // // node-sass returns relative URL paths, not file system paths From b1abe353171523f1ad087b3808546ffd5858538a Mon Sep 17 00:00:00 2001 From: Nick Petruzzelli Date: Sun, 5 Aug 2018 13:58:04 -0400 Subject: [PATCH 5/6] Remove optional support of legacy handling. Signed-off-by: Nick Petruzzelli --- README.md | 31 ------------------------- lib/loader.js | 20 ++++------------ lib/normalizeOptions.js | 51 +++++++++++------------------------------ 3 files changed, 18 insertions(+), 84 deletions(-) diff --git a/README.md b/README.md index 7634f387..bbc13b48 100644 --- a/README.md +++ b/README.md @@ -227,37 +227,6 @@ module.exports = { If you want to edit the original Sass files inside Chrome, [there's a good blog post](https://medium.com/@toolmantim/getting-started-with-css-sourcemaps-and-in-browser-sass-editing-b4daab987fb0). Checkout [test/sourceMap](https://github.com/webpack-contrib/sass-loader/tree/master/test) for a running example. -#### Opting out of legacy source maps behavior - -To receive source maps as URLs relative to `loader.resourcePath`, set the `legacySourceMaps` option to `false`. - -```javascript -module.exports = { - // ... - devtool: "source-map", - module: { - rules: [{ - test: /\.scss$/, - use: [{ - loader: "style-loader" - }, { - loader: "css-loader", options: { - sourceMap: true - } - }, { - loader: "sass-loader", options: { - legacySourceMaps: false, - sourceMap: true - } - }] - }] - } -}; -``` - -_\* If you are using a plugin to extract source maps, such as `webpack.SourceMapDevToolPlugin`, make sure to set `devtool: false`._ - - ### Environment variables If you want to prepend Sass code before the actual entry file, you can set the `data` option. In this case, the sass-loader will not override the `data` option but just append the entry's content. This is especially useful when some of your Sass variables depend on the environment: diff --git a/lib/loader.js b/lib/loader.js index e519b03c..333b545a 100644 --- a/lib/loader.js +++ b/lib/loader.js @@ -55,22 +55,10 @@ function sassLoader(content) { if (result.map && result.map !== "{}") { result.map = JSON.parse(result.map); - if (options.legacySourceMaps !== false) { - // @see https://github.com/webpack-contrib/sass-loader/issues/366#issuecomment-279460722 - result.map.sourceRoot = path.normalize(result.map.sourceRoot); - result.map.sources = result.map.sources.map(path.normalize); - - // Legacy test considers the presence of the `file` attribute to - // be an invalid source map. Until the tests are updated, - // continue with removing the property. - delete result.map.file; - } - // else { - // // node-sass returns relative URL paths, not file system paths - // // (POSIX or otherwise.) Aside from possible differences in the - // // path separator, relative URL paths are (almost always) valid - // // file system paths, allowing them to be used as such. - // } + // node-sass returns relative URL paths, not file system paths + // (POSIX or otherwise.) Aside from possible differences in the + // path separator, relative URL paths are (almost always) valid + // file system paths, allowing them to be used as such. } else { result.map = null; } diff --git a/lib/normalizeOptions.js b/lib/normalizeOptions.js index ce9071ba..e96d6fe4 100644 --- a/lib/normalizeOptions.js +++ b/lib/normalizeOptions.js @@ -78,43 +78,20 @@ function normalizeOptions(loaderContext, content, webpackImporter) { */ options.outFile = resourcePath; - if (options.legacySourceMaps !== false) { - // 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; - } - } else { - /** - * > **Special: ** Setting the `sourceMap` option requires also setting - * > the `outFile` option - * > - * > Enables the outputting of a source map during `render` and - * > `renderSync`. When `sourceMap === true`, the value of `outFile` is - * > used as the target output location for the source map. When - * > `typeof sourceMap === "string"`, the value of `sourceMap` will be - * > used as the writing location for the file. - * @see node-sass [sourceMap]{@link https://github.com/sass/node-sass#sourcemap} - * - * @type {Boolean | String | undefined} [options.sourceMap=undefined] - */ - options.sourceMap = options.outFile + ".map"; - } + /** + * > **Special: ** Setting the `sourceMap` option requires also setting + * > the `outFile` option + * > + * > Enables the outputting of a source map during `render` and + * > `renderSync`. When `sourceMap === true`, the value of `outFile` is + * > used as the target output location for the source map. When + * > `typeof sourceMap === "string"`, the value of `sourceMap` will be + * > used as the writing location for the file. + * @see node-sass [sourceMap]{@link https://github.com/sass/node-sass#sourcemap} + * + * @type {Boolean | String | undefined} [options.sourceMap=undefined] + */ + options.sourceMap = options.outFile + ".map"; } // indentedSyntax is a boolean flag. From e9fea9b5e83a142ddb8d24f6c98ce6ecdba0923e Mon Sep 17 00:00:00 2001 From: Nick Petruzzelli Date: Mon, 13 Aug 2018 17:23:22 -0400 Subject: [PATCH 6/6] Update assertions for "should produce a valid source map" test. First pass. Signed-off-by: Nick Petruzzelli --- test/index.test.js | 93 ++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 85 insertions(+), 8 deletions(-) diff --git a/test/index.test.js b/test/index.test.js index cb5130a0..b55e0f86 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -204,18 +204,95 @@ implementations.forEach(implementation => { return fakeCwd; }; + /** + * Note: this test only tests the "standard" source map + * format. It does not test the "sections" source map + * format. + * + * @see Source Map Revision 3 Proposal's [Proposed Format]{@link https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit#heading=h.qz3o9nc69um5} + */ return buildWithSourceMaps() .then(() => { + const resourcePath = path.join(__dirname, "scss", "imports.scss"); + const fileBase = path.basename(resourcePath); 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. - // Node Sass includes a duplicate entry, Dart Sass does not. - sourceMap.sources.should.have.length(implementation === nodeSass ? 11 : 10); - sourceMap.sources.forEach(sourcePath => - fs.existsSync(path.resolve(sourceMap.sourceRoot, sourcePath)) - ); + /** + * The value of the "sourceRoot" as passed to + * node-sass. + * + * @type {String|undefined|null} + */ + const sourceRootOption = undefined; + + /** + * The source map is a single JSON object. + */ + (sourceMap).should.be.an.Object(); + + /** + * Required. File version must be a positive + * integer. + */ + (sourceMap.version).should.be.a.Number().and.be.greaterThan(0); + (sourceMap.version % 1).should.equal(0); + + /** + * Optional. File is an optional name of the + * generated code that this source map is associated + * with. This is a URL string that is relative to + * source map. Typically it is just the file name + * and extension. + */ + if (sourceMap.file != null) { + (sourceMap.file).should.equal(fileBase); + } + + /** + * Optional. An optional source root, useful for + * relocating source files on a server or removing + * repeated values in the "sources" entry. This + * value is prepended to the individual entries in + * the "source" field. This is a URL string, + * undefined, or null. + */ + if (sourceRootOption != null) { + (sourceMap.sourceRoot).should.equal(sourceRootOption); + } else if (sourceMap.sourceRoot != null) { + (sourceMap.sourceRoot).should.be.a.String(); + } + + /** + * Required. Sources is an array original sources + * used by the "mappings" entry. Sources are + * URLs that are relative to the source map. + */ + (sourceMap.sources).should.be.an.Array(); + + /** + * Optional. An optional array of source content, + * useful when the "source" can’t be hosted. The + * contents are listed in the same order as the + * sources in the "sources" entry. "null" may be + * used if some original sources should be retrieved + * by name. + */ + if (sourceMap.sourcesContent != null) { + (sourceMap.sourcesContent).should.be.an.Array(); + (sourceMap.sourcesContent).should.have.length(sourceMap.sources.length); + } + + /** + * Required. Names is an array of symbol names used + * by the "mappings" entry. + */ + (sourceMap.names).should.be.an.Array(); + + /** + * Required. Mappings is a string with encoded + * mapping data. + */ + (sourceMap.mappings).should.be.a.String(); process.cwd = cwdGetter; });