Fix charset handling (#447)

* Fix @charset handling

Fixes #436
Closes #438

* Ensure @charset statements are first

index.js

@@ -107,10 +107,7 @@ function AtImport(options) {
 
         // Strip additional statements.
         bundle.forEach(stmt => {
-          if (stmt.type === "import") {
-            stmt.node.parent = undefined
-            styles.append(stmt.node)
-          } else if (stmt.type === "media") {
+          if (["charset", "import", "media"].includes(stmt.type)) {
             stmt.node.parent = undefined
             styles.append(stmt.node)
           } else if (stmt.type === "nodes") {
@@ -150,15 +147,33 @@ function AtImport(options) {
             }, Promise.resolve())
           })
           .then(() => {
+            let charset
             const imports = []
             const bundle = []
 
+            function handleCharset(stmt) {
+              if (!charset) charset = stmt
+              // charsets aren't case-sensitive, so convert to lower case to compare
+              else if (
+                stmt.node.params.toLowerCase() !==
+                charset.node.params.toLowerCase()
+              ) {
+                throw new Error(
+                  `Incompatable @charset statements:
+  ${stmt.node.params} specified in ${stmt.node.source.input.file}
+  ${charset.node.params} specified in ${charset.node.source.input.file}`
+                )
+              }
+            }
+
             // squash statements and their children
             statements.forEach(stmt => {
-              if (stmt.type === "import") {
+              if (stmt.type === "charset") handleCharset(stmt)
+              else if (stmt.type === "import") {
                 if (stmt.children) {
                   stmt.children.forEach((child, index) => {
                     if (child.type === "import") imports.push(child)
+                    else if (child.type === "charset") handleCharset(child)
                     else bundle.push(child)
                     // For better output
                     if (index === 0) child.parent = stmt
@@ -169,7 +184,9 @@ function AtImport(options) {
               }
             })
 
-            return imports.concat(bundle)
+            return charset
+              ? [charset, ...imports.concat(bundle)]
+              : imports.concat(bundle)
           })
       }
 

lib/parse-statements.js

@@ -29,6 +29,7 @@ module.exports = function (result, styles) {
     if (node.type === "atrule") {
       if (node.name === "import") stmt = parseImport(result, node)
       else if (node.name === "media") stmt = parseMedia(result, node)
+      else if (node.name === "charset") stmt = parseCharset(result, node)
     }
 
     if (stmt) {
@@ -64,20 +65,34 @@ function parseMedia(result, atRule) {
   }
 }
 
+function parseCharset(result, atRule) {
+  if (atRule.prev()) {
+    return result.warn("@charset must precede all other statements", {
+      node: atRule,
+    })
+  }
+  return {
+    type: "charset",
+    node: atRule,
+    media: [],
+  }
+}
+
 function parseImport(result, atRule) {
-  let prev = getPrev(atRule)
+  let prev = atRule.prev()
   if (prev) {
     do {
       if (
-        prev.type !== "atrule" ||
-        (prev.name !== "import" && prev.name !== "charset")
+        prev.type !== "comment" &&
+        (prev.type !== "atrule" ||
+          (prev.name !== "import" && prev.name !== "charset"))
       ) {
         return result.warn(
           "@import must precede all other statements (besides @charset)",
           { node: atRule }
         )
       }
-      prev = getPrev(prev)
+      prev = prev.prev()
     } while (prev)
   }
 
@@ -128,11 +143,3 @@ function parseImport(result, atRule) {
 
   return stmt
 }
-
-function getPrev(item) {
-  let prev = item.prev()
-  while (prev && prev.type === "comment") {
-    prev = prev.prev()
-  }
-  return prev
-}

test/fixtures/charset-error.css

@@ -0,0 +1,2 @@
+@charset "foobar";
+@import "imports/charset.css";

test/fixtures/charset-import.css

@@ -0,0 +1,4 @@
+@charset "UTF-8";
+@import "test/fixtures/imports/foo.css";
+@import "test/fixtures/imports/charset.css";
+bar{}

test/fixtures/charset-import.expected.css

@@ -0,0 +1,3 @@
+@charset "UTF-8";
+foo{}
+bar{}

test/fixtures/imports/charset.css

@@ -0,0 +1 @@
+@charset "UTF-8";

test/import.js

@@ -46,6 +46,38 @@ test("should not fail with absolute and local import", t => {
     .then(result => t.is(result.css, "@import url('http://');\nfoo{}"))
 })
 
+test("should keep @charset first", t => {
+  const base = '@charset "UTF-8";\n@import url(http://);'
+  return postcss()
+    .use(atImport())
+    .process(base, { from: undefined })
+    .then(result => {
+      t.is(result.warnings().length, 0)
+      t.is(result.css, base)
+    })
+})
+
+test(
+  "should handle multiple @charset statements",
+  checkFixture,
+  "charset-import"
+)
+
+test("should error if incompatable @charset statements", t => {
+  t.plan(2)
+  const file = "test/fixtures/charset-error.css"
+  return postcss()
+    .use(atImport())
+    .process(readFileSync(file), { from: file })
+    .catch(err => {
+      t.truthy(err)
+      t.regex(
+        err.message,
+        /Incompatable @charset statements:.+specified in.+specified in.+/s
+      )
+    })
+})
+
 test("should error when file not found", t => {
   t.plan(1)
   const file = "test/fixtures/imports/import-missing.css"

test/lint.js

@@ -78,6 +78,23 @@ test("should not warn when @charset or @import statement before", t => {
   })
 })
 
+test("should warn when @charset is not first", t => {
+  return Promise.all([
+    processor.process(`a {} @charset "utf-8";`, { from: undefined }),
+    processor.process(`@media {} @charset "utf-8";`, { from: undefined }),
+    processor.process(`/* foo */ @charset "utf-8";`, { from: undefined }),
+    processor.process(`@import "bar.css"; @charset "utf-8";`, {
+      from: "test/fixtures/imports/foo.css",
+    }),
+  ]).then(results => {
+    results.forEach(result => {
+      const warnings = result.warnings()
+      t.is(warnings.length, 1)
+      t.is(warnings[0].text, "@charset must precede all other statements")
+    })
+  })
+})
+
 test("should warn when a user didn't close an import with ;", t => {
   return processor
     .process(`@import url('http://') :root{}`, { from: undefined })