fix: strip BOM before processing with postcss (#1684)

postcss stripped a leading BOM until 8.5.24 and preserves it since, so a
BOM added by an earlier loader (sass-loader with `charset: true`) survived
into the module and landed mid-file once stylesheets were concatenated,
silently dropping the rule after it.

Strip it for both inputs postcss accepts: the string, and the `Root` a
previous loader hands over as `meta.ast`, where the BOM lives on
`source.input.hasBOM` rather than in the tree.

Author: alexander-akait

src/index.js

@@ -28,6 +28,7 @@ import {
   warningFactory,
   syntaxErrorFactory,
   supportTemplateLiteral,
+  stripBom,
 } from "./utils";
 
 export default async function loader(content, map, meta) {
@@ -174,7 +175,7 @@ export default async function loader(content, map, meta) {
   let result;
 
   try {
-    result = await postcss(plugins).process(content, {
+    result = await postcss(plugins).process(stripBom(content), {
       hideNothingWarning: true,
       from: resourcePath,
       to: resourcePath,

src/utils.js

@@ -1471,6 +1471,27 @@ function supportTemplateLiteral(loaderContext) {
   return false;
 }
 
+const BOM_CODE_POINT = 0xfeff;
+
+// postcss stripped a leading BOM until 8.5.24 and preserves it since, so strip
+// it ourselves: concatenated stylesheets must not carry one in the middle.
+// `content` is a postcss `Root` when a previous loader handed over its AST,
+// and there the BOM lives on the input rather than in the tree.
+function stripBom(content) {
+  if (typeof content === "string") {
+    return content.charCodeAt(0) === BOM_CODE_POINT
+      ? content.slice(1)
+      : content;
+  }
+
+  if (content && content.source && content.source.input) {
+    // eslint-disable-next-line no-param-reassign
+    content.source.input.hasBOM = false;
+  }
+
+  return content;
+}
+
 export {
   normalizeOptions,
   shouldUseModulesPlugins,
@@ -1499,4 +1520,5 @@ export {
   warningFactory,
   syntaxErrorFactory,
   supportTemplateLiteral,
+  stripBom,
 };

test/__snapshots__/loader.test.js.snap

@@ -63,6 +63,39 @@ exports[`loader should not generate console.warn when plugins disabled and hideN
 
 exports[`loader should not generate console.warn when plugins disabled and hideNothingWarning is "true": warnings 1`] = `[]`;
 
+exports[`loader should not keep a BOM in an ast reused from a previous loader: errors 1`] = `[]`;
+
+exports[`loader should not keep a BOM in an ast reused from a previous loader: warnings 1`] = `[]`;
+
+exports[`loader should not pass a BOM added by a previous loader to postcss: errors 1`] = `[]`;
+
+exports[`loader should not pass a BOM added by a previous loader to postcss: module 1`] = `
+"// Imports
+import ___CSS_LOADER_API_NO_SOURCEMAP_IMPORT___ from "../../../../src/runtime/noSourceMaps.js";
+import ___CSS_LOADER_API_IMPORT___ from "../../../../src/runtime/api.js";
+var ___CSS_LOADER_EXPORT___ = ___CSS_LOADER_API_IMPORT___(___CSS_LOADER_API_NO_SOURCEMAP_IMPORT___);
+// Module
+___CSS_LOADER_EXPORT___.push([module.id, \`.first::after {
+  content: "©";
+}
+\`, ""]);
+// Exports
+export default ___CSS_LOADER_EXPORT___;
+"
+`;
+
+exports[`loader should not pass a BOM added by a previous loader to postcss: result 1`] = `
+".first::after {
+  content: "©";
+}
+.second::after {
+  content: "→";
+}
+"
+`;
+
+exports[`loader should not pass a BOM added by a previous loader to postcss: warnings 1`] = `[]`;
+
 exports[`loader should pass queries to other loader: errors 1`] = `[]`;
 
 exports[`loader should pass queries to other loader: module 1`] = `

test/fixtures/modules/issue-1678/first.css

@@ -0,0 +1,3 @@
+.first::after {
+  content: "©";
+}

test/fixtures/modules/issue-1678/second.css

@@ -0,0 +1,3 @@
+.second::after {
+  content: "→";
+}

test/fixtures/modules/issue-1678/source.js

@@ -0,0 +1,9 @@
+import first from "./first.css";
+import second from "./second.css";
+
+// Concatenation is where a preserved BOM corrupts: it lands mid-file.
+const css = first.toString() + second.toString();
+
+__export__ = css;
+
+export default css;

test/fixtures/modules/issue-1678/with-bom-loader.js

@@ -0,0 +1,3 @@
+// Emulates sass-loader with `charset: true`, which prefixes a BOM when the
+// stylesheet is non-ASCII.
+module.exports = (content) => "\uFEFF" + content;

test/loader.test.js

@@ -1,5 +1,6 @@
 import path from "path";
 
+import postcss from "postcss";
 import postcssPresetEnv from "postcss-preset-env";
 
 import {
@@ -207,6 +208,84 @@ describe("loader", () => {
     expect(getErrors(stats)).toMatchSnapshot("errors");
   });
 
+  it("should not pass a BOM added by a previous loader to postcss", async () => {
+    const BOM = "\uFEFF";
+    const processSpy = jest.spyOn(Object.getPrototypeOf(postcss()), "process");
+    const compiler = getCompiler(
+      "./modules/issue-1678/source.js",
+      {},
+      {
+        module: {
+          rules: [
+            {
+              test: /\.css$/i,
+              use: [
+                { loader: path.resolve(__dirname, "../src") },
+                { loader: "./modules/issue-1678/with-bom-loader.js" },
+              ],
+            },
+          ],
+        },
+      },
+    );
+
+    const stats = await compile(compiler);
+    const executed = getExecutedCode("main.bundle.js", compiler, stats);
+
+    expect(processSpy).toHaveBeenCalledTimes(2);
+
+    for (const [css] of processSpy.mock.calls) {
+      expect(css.startsWith(BOM)).toBe(false);
+    }
+
+    // postcss >= 8.5.24 keeps the BOM, so without stripping it lands between
+    // the two stylesheets and a browser drops the rule after it.
+    expect(executed).not.toContain(BOM);
+    expect(executed).toContain(".first::after");
+    expect(executed).toContain(".second::after");
+
+    expect(
+      getModuleSource("./modules/issue-1678/first.css", stats),
+    ).toMatchSnapshot("module");
+    expect(executed).toMatchSnapshot("result");
+    expect(getWarnings(stats)).toMatchSnapshot("warnings");
+    expect(getErrors(stats)).toMatchSnapshot("errors");
+
+    processSpy.mockRestore();
+  });
+
+  it("should not keep a BOM in an ast reused from a previous loader", async () => {
+    const BOM = "\uFEFF";
+    const compiler = getCompiler(
+      "./modules/issue-1678/source.js",
+      {},
+      {
+        module: {
+          rules: [
+            {
+              test: /\.css$/i,
+              use: [
+                { loader: path.resolve(__dirname, "../src") },
+                { loader: require.resolve("./helpers/ast-loader") },
+                { loader: "./modules/issue-1678/with-bom-loader.js" },
+              ],
+            },
+          ],
+        },
+      },
+    );
+
+    const stats = await compile(compiler);
+    const executed = getExecutedCode("main.bundle.js", compiler, stats);
+
+    expect(executed).not.toContain(BOM);
+    expect(executed).toContain(".first::after");
+    expect(executed).toContain(".second::after");
+
+    expect(getWarnings(stats)).toMatchSnapshot("warnings");
+    expect(getErrors(stats)).toMatchSnapshot("errors");
+  });
+
   it('should work with "sass-loader"', async () => {
     const compiler = getCompiler(
       "./scss/source.js",