Merge pull request #117 from bholloway/remove-orphan-CR-character

Remove orphan CR characters

Author: bholloway

packages/resolve-url-loader/README.md

@@ -110,16 +110,17 @@ Refer to `test` directory for full webpack configurations (as used in automated
 
 ## Options
 
-| option     | type                       | default     |          |  description                                                                                                                                                                     |
-|------------|----------------------------|-------------|----------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
-|`engine`    | `'rework'`<br/>`'postcss'` | `'postcss'` |          | The css parser engine.                                                                                                                                                           |
-|`sourceMap` | boolean                    | `false`     |          | Generate a source-map.                                                                                                                                                           |
-|`keepQuery` | boolean                    | `false`     |          | Keep query-string and/or hash suffixes.<br/>e.g. `url('./MyFont.eot?#iefix')`<br/>Be aware downstream loaders may remove query-string or hash.                                   |
-|`debug`     | boolean                    | `false`     |          | Display debug information.                                                                                                                                                       |
-|`silent`    | boolean                    | `false`     |          | Do **not** display warnings.                                                                                                                                                     |
-|`root`      | string                     | _unset_     |          | Similar to the (now defunct) option in `css-loader`.<br/>This string, possibly empty, is prepended to absolute URIs.<br/>Absolute URIs are only processed if this option is set. |
-|`join`      | function                   | _inbuilt_   | advanced | Custom join function.<br/>Use custom javascript to fix asset paths on a per-case basis.<br/>Refer to the default implementation for more information.                            |
-|`absolute`  | boolean                    | `false`     | useless  | Forces URIs to be output as absolute file paths.<br/>This is retained for historical compatibility but is likely to be removed in the future, so let me know if you use it.      |
+| option      | type                       | default     |          |  description                                                                                                                                                                     |
+|-------------|----------------------------|-------------|----------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| `engine`    | `'rework'`<br/>`'postcss'` | `'postcss'` |          | The css parser engine.                                                                                                                                                           |
+| `sourceMap` | boolean                    | `false`     |          | Generate a source-map.                                                                                                                                                           |
+| `keepQuery` | boolean                    | `false`     |          | Keep query-string and/or hash suffixes.<br/>e.g. `url('./MyFont.eot?#iefix')`<br/>Be aware downstream loaders may remove query-string or hash.                                   |
+| `removeCR`  | boolean                    | `false`     |          | Convert orphan CR to whitespace (postcss only).<br/>See known issues below.                                                                                                          |
+| `debug`     | boolean                    | `false`     |          | Display debug information.                                                                                                                                                       |
+| `silent`    | boolean                    | `false`     |          | Do **not** display warnings.                                                                                                                                                     |
+| `root`      | string                     | _unset_     |          | Similar to the (now defunct) option in `css-loader`.<br/>This string, possibly empty, is prepended to absolute URIs.<br/>Absolute URIs are only processed if this option is set. |
+| `join`      | function                   | _inbuilt_   | advanced | Custom join function.<br/>Use custom javascript to fix asset paths on a per-case basis.<br/>Refer to the default implementation for more information.                            |
+| `absolute`  | boolean                    | `false`     | useless  | Forces URIs to be output as absolute file paths.<br/>This is retained for historical compatibility but is likely to be removed in the future, so let me know if you use it.      |
 
 ## How it works
 
@@ -160,6 +161,8 @@ All `webpack1`-`webpack4` with contemporaneous loaders/plugins.
 
 Refer to `test` directory for full webpack configurations (as used in automated tests).
 
+Some edge cases with `libsass` on `Windows` (see below).
+
 ### Engines
 
 The `engine:postcss` is by far the more reliable option.
@@ -180,6 +183,35 @@ However recall that any paths that _are_ processed will have windows back-slash
 
 It can also be useful to process absolute URIs if you have a custom `join` function and want to process all paths. However this is perhaps better done with some separate `postcss` plugin.
 
+### Windows line breaks
+
+Normal windows linebreaks are `CRLF`. But sometimes libsass will output single `CR` characters.
+
+This problem is specific to multiline declarations. Refer to the [libsass bug #2693](https://github.com/sass/libsass/issues/2693).
+
+If you have _any_ such multiline declarations preceding `url()` statements it will fail your build.
+ 
+Libsass doesn't consider these orphan `CR` to be newlines but `postcss` engine does.  The result being an offset in source-map line-numbers which crashes `resolve-url-loader`.
+
+```
+Module build failed: Error: resolve-url-loader: CSS error
+  source-map information is not available at url() declaration
+```
+
+Some users find the node-sass `linefeed` option solves the problem.
+
+**Solutions**
+* Try the node-sass [linefeed](https://github.com/sass/node-sass#linefeed--v300) option by way of `sass-loader`.
+
+**Work arounds** 
+* Enable `removeCR` option [here](#option).
+* Remove linebreaks in declarations.
+
+**Diagnosis**
+1. Run a stand-alone sass build `npx node-sass index.scss output.css`
+2. Use a hex editor to check line endings `Format-Hex output.css` 
+3. Expect `0DOA` (or desired) line endings. Single `0D` confirms this problem.
+
 ## Getting help
 
 Webpack is difficult to configure but extremely rewarding.

packages/resolve-url-loader/index.js

@@ -50,6 +50,7 @@ function resolveUrlLoader(content, sourceMap) {
       silent   : false,
       absolute : false,
       keepQuery: false,
+      removeCR : false,
       root     : false,
       debug    : false,
       join     : joinFn.defaultJoin
@@ -165,7 +166,8 @@ function resolveUrlLoader(content, sourceMap) {
       outputSourceMap     : !!options.sourceMap,
       transformDeclaration: valueProcessor(loader.resourcePath, options),
       absSourceMap        : absSourceMap,
-      sourceMapConsumer   : sourceMapConsumer
+      sourceMapConsumer   : sourceMapConsumer,
+      removeCR            : options.removeCR
     }))
     .catch(onFailure)
     .then(onSuccess);

packages/resolve-url-loader/lib/engine/postcss.js

@@ -4,27 +4,34 @@
  */
 'use strict';
 
-var path    = require('path'),
+var os      = require('os'),
+    path    = require('path'),
     postcss = require('postcss');
 
 var fileProtocol = require('../file-protocol');
 
+var ORPHAN_CR_REGEX = /\r(?!\n)(.|\n)?/g;
+
 /**
  * Process the given CSS content into reworked CSS content.
  *
  * @param {string} sourceFile The absolute path of the file being processed
  * @param {string} sourceContent CSS content without source-map
  * @param {{outputSourceMap: boolean, transformDeclaration:function, absSourceMap:object,
- *        sourceMapConsumer:object}} params Named parameters
+ *        sourceMapConsumer:object, removeCR:boolean}} params Named parameters
  * @return {{content: string, map: object}} Reworked CSS and optional source-map
  */
 function process(sourceFile, sourceContent, params) {
+  // #107 libsass emits orphan CR not considered newline, postcss does consider newline (content vs source-map mismatch)
+  var correctedContent = params.removeCR && (os.EOL !== '\r') ?
+    sourceContent.replace(ORPHAN_CR_REGEX, ' $1') :
+    sourceContent;
 
   // prepend file protocol to all sources to avoid problems with source map
   return postcss([
     postcss.plugin('postcss-resolve-url', postcssPlugin)
   ])
-    .process(sourceContent, {
+    .process(correctedContent, {
       from: fileProtocol.prepend(sourceFile),
       map : params.outputSourceMap && {
         prev          : !!params.absSourceMap && fileProtocol.prepend(params.absSourceMap),
@@ -69,7 +76,10 @@ function process(sourceFile, sourceContent, params) {
         }
         // source-map present but invalid entry
         else if (params.sourceMapConsumer) {
-          throw new Error('source-map information is not available at url() declaration');
+          throw new Error(
+            'source-map information is not available at url() declaration ' +
+            (ORPHAN_CR_REGEX.test(sourceContent) ? '(found orphan CR, try removeCR option)' : '(no orphan CR found)')
+          );
         }
       }
     }

packages/resolve-url-loader/package.json

@@ -1,6 +1,6 @@
 {
   "name": "resolve-url-loader",
-  "version": "3.0.1",
+  "version": "3.1.0-beta.2",
   "description": "Webpack loader that resolves relative paths in url() statements based on the original source file",
   "main": "index.js",
   "repository": {

test/cases/common/tests.js

@@ -101,6 +101,20 @@ exports.testKeepQuery = (...rest) =>
     )
   );
 
+exports.testRemoveCR = (...rest) =>
+  test(
+    'removeCR=true',
+    layer()(
+      env({
+        LOADER_QUERY: 'removeCR',
+        LOADER_OPTIONS: {removeCR: true},
+        OUTPUT: 'remove-CR'
+      }),
+      ...rest,
+      test('validate', assertStderr('options.removeCR')(1)`removeCR: true`)
+    )
+  );
+
 exports.testRoot = (...rest) =>
   test(
     'root=empty',

test/cases/orphan-carriage-return.postcss.js

@@ -0,0 +1,101 @@
+'use strict';
+
+const {join} = require('path');
+const outdent = require('outdent');
+const {test, layer, fs, env, cwd} = require('test-my-cli');
+
+const {withCacheBase} = require('../lib/higher-order');
+const {testDefault, testRemoveCR} = require('./common/tests');
+const {
+  buildDevNormal, buildDevBail, buildDevNoUrl, buildProdNormal, buildProdBail, buildProdNoUrl, buildProdNoDevtool
+} = require('./common/builds');
+const {
+  onlyMeta, assertWebpackOk, assertWebpackNotOk, assertStdout, assertNoErrors, assertNoMessages
+} = require('../lib/assert');
+
+// Allow 1-4 errors
+//  - known-issue in extract-text-plugin, failed loaders will rerun webpack>=2
+//  - webpack may repeat errors with a header line taken from the parent loader
+const assertCssError = assertStdout('error')([1, 4])`
+  ^[ ]*ERROR[^\n]*
+  ([^\n]+\n){0,2}[^\n]*resolve-url-loader:[ ]*CSS error
+  [ ]+source-map information is not available at url\(\) declaration \(found orphan CR, try removeCR option\)
+  `;
+
+module.exports = test(
+  'orphan-carriage-return',
+  layer('orphan-carriage-return')(
+    cwd('.'),
+    fs({
+      'package.json': withCacheBase('package.json'),
+      'webpack.config.js': withCacheBase('webpack.config.js'),
+      'node_modules': withCacheBase('node_modules'),
+      'src/index.scss': outdent`
+        .some-class-name {
+          font-size: calc(${'\r'}
+            (1px)${'\r'}
+          );
+          background-image: url(data:image/gif;base64,R0lGODlhAQABAIAAAP///wAAACH5BAEAAAAALAAAAAABAAEAAAICRAEAOw==);
+        }
+        `
+    }),
+    env({
+      ENTRY: join('src', 'index.scss')
+    }),
+    testDefault(
+      onlyMeta('meta.version.webpack == 1')(
+        buildDevBail(
+          assertWebpackNotOk
+        ),
+        buildDevNormal(
+          assertWebpackOk,
+          assertCssError
+        ),
+        buildProdBail(
+          assertWebpackNotOk
+        ),
+        buildProdNormal(
+          assertWebpackOk,
+          assertCssError
+        )
+      ),
+      onlyMeta('meta.version.webpack > 1')(
+        buildDevNormal(
+          assertWebpackNotOk,
+          assertCssError
+        ),
+        buildProdNormal(
+          assertWebpackNotOk,
+          assertCssError
+        )
+      )
+    ),
+    testRemoveCR(
+      buildDevNormal(
+        assertWebpackOk,
+        assertNoErrors,
+        assertNoMessages
+      ),
+      buildDevNoUrl(
+        assertWebpackOk,
+        assertNoErrors,
+        assertNoMessages
+      ),
+      buildProdNormal(
+        assertWebpackOk,
+        assertNoErrors,
+        assertNoMessages
+      ),
+      buildProdNoUrl(
+        assertWebpackOk,
+        assertNoErrors,
+        assertNoMessages
+      ),
+      buildProdNoDevtool(
+        assertWebpackOk,
+        assertNoErrors,
+        assertNoMessages
+      )
+    )
+  )
+);