Repository: postcss/postcss-import
Pull request #535 (2023-07-20 16:24:59)

check import cycles
resolves : https://github.com/postcss/postcss-import/issues/462

It is untrue that browsers only use the last import.
They use all of them.

Up until now however this wasn't observable.

With the introduction of `@layer` CSS now has something that is registered when first encountered and can not be overwritten later, making the real behavior clear.

When `skipDuplicates: false` is set `postcss-import` will produce CSS that is equivalent to browser behavior.

This change adds handling of cyclical imports in the same way as browsers resolve these cycles. This removes any differences between bundled or unbundled CSS as affected by import order.

-----

See WebKit implementation : https://github.com/WebKit/WebKit/blob/main/Source/WebCore/css/StyleRuleImport.cpp#L123-L131


Patch:
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
index 209586c..ca3a969 100644
--- a/.github/workflows/ci.yaml
+++ b/.github/workflows/ci.yaml
@@ -12,7 +12,7 @@ jobs:
       matrix:
         node-version: [14.x, 16.x, 18.x]
     steps:
-    - uses: actions/checkout@v3
+    - uses: actions/checkout@v4
     - name: Use Node.js ${{ matrix.node-version }}
       uses: actions/setup-node@v3
       with:
diff --git a/README.md b/README.md
index b698874..c7e4081 100644
--- a/README.md
+++ b/README.md
@@ -125,7 +125,7 @@ Checkout the [tests](test) for more examples.
 
 ### Options
 
-### `filter`
+#### `filter`
 Type: `Function`  
 Default: `() => true`
 
@@ -164,7 +164,7 @@ Type: `Function`
 Default: `null`
 
 You can provide a custom path resolver with this option. This function gets
-`(id, basedir, importOptions)` arguments and should return a path, an array of
+`(id, basedir, importOptions, astNode)` arguments and should return a path, an array of
 paths or a promise resolving to the path(s). If you do not return an absolute
 path, your path will be resolved to an absolute path using the default
 resolver.
@@ -204,15 +204,23 @@ this value will be ignored.
 
 #### `nameLayer`
 
-Type: `Function`
+Type: `Function`  
 Default: `null`
 
-You can provide a custom naming function for anonymous layers (`@import 'baz.css' layer;`).
+You can provide a custom naming function for anonymous layers (`@import 'baz.css' layer;`).  
 This function gets `(index, rootFilename)` arguments and should return a unique string.
 
-This option only influences imports without a layer name.
+This option only influences imports without a layer name.  
 Without this option the plugin will warn on anonymous layers.
 
+#### `warnOnEmpty`
+
+Type: `Boolean`  
+Default: `true`
+
+By default `postcss-import` warns when an empty file is imported.  
+Set this option to `false` to disable this warning.
+
 #### Example with some options
 
 ```js
diff --git a/index.js b/index.js
index 1871a63..59ed776 100755
--- a/index.js
+++ b/index.js
@@ -20,6 +20,7 @@ function AtImport(options) {
     plugins: [],
     addModulesDirectories: [],
     nameLayer: null,
+    warnOnEmpty: true,
     ...options,
   }
 
@@ -62,7 +63,7 @@ function AtImport(options) {
         state,
         [],
         [],
-        "",
+        [],
         postcss
       )
 
diff --git a/lib/join-media.js b/lib/join-media.js
index 0eff3c9..04ee827 100644
--- a/lib/join-media.js
+++ b/lib/join-media.js
@@ -20,6 +20,8 @@ module.exports = function joinMedia(parentMedia, childMedia) {
         } else {
           media.push(`${parentItem} and ${childItem}`)
         }
+      } else {
+        media.push(parentItem)
       }
     })
   })
diff --git a/lib/parse-styles.js b/lib/parse-styles.js
index d021dd4..6aaa245 100644
--- a/lib/parse-styles.js
+++ b/lib/parse-styles.js
@@ -27,8 +27,7 @@ async function parseStyles(
     stmt.parentMedia = media
     stmt.layer = joinLayer(layer, stmt.layer || [])
 
-    // skip protocol base uri (protocol://url) or protocol-relative
-    if (stmt.type !== "import" || /^(?:[a-z]+:)?\/\//i.test(stmt.uri)) {
+    if (stmt.type !== "import" || !isProcessableURL(stmt.uri)) {
       continue
     }
 
@@ -50,8 +49,8 @@ async function parseStyles(
     else if (
       stmt.node.params.toLowerCase() !== charset.node.params.toLowerCase()
     ) {
-      throw new Error(
-        `Incompatable @charset statements:
+      throw stmt.node.error(
+        `Incompatible @charset statements:
   ${stmt.node.params} specified in ${stmt.node.source.input.file}
   ${charset.node.params} specified in ${charset.node.source.input.file}`
       )
@@ -92,7 +91,7 @@ async function resolveImportId(result, stmt, options, state, postcss) {
     )
 
     return
-  } else if (dataURL.isValid(stmt.from)) {
+  } else if (dataURL.isValid(stmt.from.slice(-1))) {
     // Data urls can't be used a base url to resolve imports.
     // When the parent statement has a data url
     // and the current statement doesn't have a data url we ignore the statement.
@@ -108,12 +107,14 @@ async function resolveImportId(result, stmt, options, state, postcss) {
     ? path.dirname(atRule.source.input.file)
     : options.root
 
-  const paths = [await options.resolve(stmt.uri, base, options)].flat()
+  const paths = [await options.resolve(stmt.uri, base, options, atRule)].flat()
 
   // Ensure that each path is absolute:
   const resolved = await Promise.all(
     paths.map(file => {
-      return !path.isAbsolute(file) ? resolveId(file, base, options) : file
+      return !path.isAbsolute(file)
+        ? resolveId(file, base, options, atRule)
+        : file
     })
   )
 
@@ -147,7 +148,7 @@ async function loadImportContent(
   postcss
 ) {
   const atRule = stmt.node
-  const { media, layer } = stmt
+  const { media, layer, from } = stmt
 
   assignLayerNames(layer, atRule, state, options)
 
@@ -167,9 +168,13 @@ async function loadImportContent(
     state.importedFiles[filename][media][layer] = true
   }
 
+  if (from.includes(filename)) {
+    return
+  }
+
   const content = await options.load(filename, options)
 
-  if (content.trim() === "") {
+  if (content.trim() === "" && options.warnOnEmpty) {
     result.warn(`${filename} is empty`, { node: atRule })
     return
   }
@@ -214,9 +219,32 @@ async function loadImportContent(
     state,
     media,
     layer,
-    filename,
+    [...from, filename],
     postcss
   )
 }
 
+function isProcessableURL(uri) {
+  // skip protocol base uri (protocol://url) or protocol-relative
+  if (/^(?:[a-z]+:)?\/\//i.test(uri)) {
+    return false
+  }
+
+  // check for fragment or query
+  try {
+    // needs a base to parse properly
+    const url = new URL(uri, "https://example.com")
+
+    if (url.hash) {
+      return false
+    }
+
+    if (url.search) {
+      return false
+    }
+  } catch {} // Ignore
+
+  return true
+}
+
 module.exports = parseStyles
diff --git a/lib/resolve-id.js b/lib/resolve-id.js
index d6d1668..aca3002 100644
--- a/lib/resolve-id.js
+++ b/lib/resolve-id.js
@@ -11,7 +11,7 @@ function resolveModule(id, opts) {
   })
 }
 
-module.exports = function resolveId(id, base, options) {
+module.exports = function resolveId(id, base, options, node) {
   const paths = options.path
 
   const resolveOpts = {
@@ -32,10 +32,10 @@ module.exports = function resolveId(id, base, options) {
     .catch(() => {
       if (paths.indexOf(base) === -1) paths.unshift(base)
 
-      throw new Error(
+      throw node.error(
         `Failed to find '${id}'
   in [
-    ${paths.join(",\n        ")}
+    ${paths.join(",\n    ")}
   ]`
       )
     })


Test patch:
diff --git a/test/fixtures/cyclical-skip-duplicates.css b/test/fixtures/cyclical-skip-duplicates.css
new file mode 100644
index 0000000..4dc4d4a
--- /dev/null
+++ b/test/fixtures/cyclical-skip-duplicates.css
@@ -0,0 +1,4 @@
+@import "cyclical-a.css";
+@import "cyclical-b.css";
+
+@import "cyclical-screen.css" screen;
diff --git a/test/fixtures/cyclical-skip-duplicates.expected.css b/test/fixtures/cyclical-skip-duplicates.expected.css
new file mode 100644
index 0000000..24c2d5e
--- /dev/null
+++ b/test/fixtures/cyclical-skip-duplicates.expected.css
@@ -0,0 +1,23 @@
+
+
+.b {
+  color: red;
+}
+
+.a {
+  color: blue;
+}
+
+@media screen and all {
+
+.a {
+  color: aquamarine;
+}
+}
+
+@media screen {
+
+.a {
+  color: pink;
+}
+}
diff --git a/test/fixtures/cyclical.css b/test/fixtures/cyclical.css
new file mode 100644
index 0000000..4dc4d4a
--- /dev/null
+++ b/test/fixtures/cyclical.css
@@ -0,0 +1,4 @@
+@import "cyclical-a.css";
+@import "cyclical-b.css";
+
+@import "cyclical-screen.css" screen;
diff --git a/test/fixtures/cyclical.expected.css b/test/fixtures/cyclical.expected.css
new file mode 100644
index 0000000..5779150
--- /dev/null
+++ b/test/fixtures/cyclical.expected.css
@@ -0,0 +1,31 @@
+
+
+.b {
+  color: red;
+}
+
+.a {
+  color: blue;
+}
+
+.a {
+  color: blue;
+}
+
+.b {
+  color: red;
+}
+
+@media screen and all {
+
+.a {
+  color: aquamarine;
+}
+}
+
+@media screen {
+
+.a {
+  color: pink;
+}
+}
diff --git a/test/fixtures/filter-ignore.css b/test/fixtures/filter-ignore.css
index d134503..923d64d 100644
--- a/test/fixtures/filter-ignore.css
+++ b/test/fixtures/filter-ignore.css
@@ -12,4 +12,6 @@
 @import url("//css");
 @import url('//css');
 @import url(//css);
+@import url('foo.css?query=string');
+@import url('foo.css#hash');
 content{}
diff --git a/test/fixtures/filter-ignore.expected.css b/test/fixtures/filter-ignore.expected.css
index a979122..03093e4 100644
--- a/test/fixtures/filter-ignore.expected.css
+++ b/test/fixtures/filter-ignore.expected.css
@@ -13,6 +13,8 @@
 @import url("//css");
 @import url('//css');
 @import url(//css);
+@import url('foo.css?query=string');
+@import url('foo.css#hash');
 @media (min-width: 25em){
 ignore{}
 }
diff --git a/test/fixtures/imports/cyclical-a.css b/test/fixtures/imports/cyclical-a.css
new file mode 100644
index 0000000..ec5adb4
--- /dev/null
+++ b/test/fixtures/imports/cyclical-a.css
@@ -0,0 +1,5 @@
+@import url(cyclical-b.css);
+
+.a {
+  color: blue;
+}
diff --git a/test/fixtures/imports/cyclical-all.css b/test/fixtures/imports/cyclical-all.css
new file mode 100644
index 0000000..f1b5ab1
--- /dev/null
+++ b/test/fixtures/imports/cyclical-all.css
@@ -0,0 +1,5 @@
+@import url(cyclical-screen.css) screen;
+
+.a {
+  color: aquamarine;
+}
diff --git a/test/fixtures/imports/cyclical-b.css b/test/fixtures/imports/cyclical-b.css
new file mode 100644
index 0000000..8452916
--- /dev/null
+++ b/test/fixtures/imports/cyclical-b.css
@@ -0,0 +1,5 @@
+@import url(cyclical-a.css);
+
+.b {
+  color: red;
+}
diff --git a/test/fixtures/imports/cyclical-screen.css b/test/fixtures/imports/cyclical-screen.css
new file mode 100644
index 0000000..1e51af7
--- /dev/null
+++ b/test/fixtures/imports/cyclical-screen.css
@@ -0,0 +1,5 @@
+@import url(cyclical-all.css) all;
+
+.a {
+  color: pink;
+}
diff --git a/test/fixtures/imports/media-query-screen.css b/test/fixtures/imports/media-query-screen.css
new file mode 100644
index 0000000..e96a63d
--- /dev/null
+++ b/test/fixtures/imports/media-query-screen.css
@@ -0,0 +1,1 @@
+@import "foo.css" screen;
diff --git a/test/fixtures/media-join.css b/test/fixtures/media-join.css
index b9d595a..28ca38a 100644
--- a/test/fixtures/media-join.css
+++ b/test/fixtures/media-join.css
@@ -1,3 +1,4 @@
 @import "media-join-nested.css" one-1;
 @import "media-join-nested.css" or-left-1, or-right-1;
 @import "media-join-nested.css" and-left-1 and and-right-1;
+@import "media-query-screen.css" screen;
diff --git a/test/fixtures/media-join.expected.css b/test/fixtures/media-join.expected.css
index 0dd4562..b733b0b 100644
--- a/test/fixtures/media-join.expected.css
+++ b/test/fixtures/media-join.expected.css
@@ -15,3 +15,7 @@
 @media and-left-1 and and-right-1 and or-left-2, and-left-1 and and-right-1 and or-right-2 {}
 
 @media and-left-1 and and-right-1 and and-left-2 and and-right-2 {}
+
+@media screen {
+foo{}
+}
diff --git a/test/helpers/check-fixture.js b/test/helpers/check-fixture.js
index 480d04b..de66739 100644
--- a/test/helpers/check-fixture.js
+++ b/test/helpers/check-fixture.js
@@ -39,5 +39,13 @@ module.exports = function (t, file, opts, postcssOpts, warnings) {
           `unexpected warning: "${warning.text}"`
         )
       })
+
+      t.is(
+        warnings.length,
+        result.warnings().length,
+        `expected ${warnings.length} warning(s), got ${
+          result.warnings().length
+        }`
+      )
     })
 }
diff --git a/test/import.js b/test/import.js
index c13bd76..e0d0b8a 100644
--- a/test/import.js
+++ b/test/import.js
@@ -13,15 +13,38 @@ const atImport = require("..")
 // internal tooling
 const checkFixture = require("./helpers/check-fixture")
 
-test("should import stylsheets", checkFixture, "simple")
+test("should import stylesheets", checkFixture, "simple")
 
-test("should not import a stylsheet twice", checkFixture, "no-duplicate")
+test("should not import a stylesheet twice", checkFixture, "no-duplicate")
 
-test("should be able to import a stylsheet twice", checkFixture, "duplicates", {
-  skipDuplicates: false,
-})
+test(
+  "should be able to import a stylesheet twice",
+  checkFixture,
+  "duplicates",
+  {
+    skipDuplicates: false,
+  }
+)
 
-test("should import stylsheets with same content", checkFixture, "same")
+test(
+  "should be able to import a stylesheet with cyclical dependencies",
+  checkFixture,
+  "cyclical",
+  {
+    skipDuplicates: false,
+  }
+)
+
+test(
+  "should be able to import a stylesheet with cyclical dependencies and skip duplicates is true",
+  checkFixture,
+  "cyclical-skip-duplicates",
+  {
+    skipDuplicates: true,
+  }
+)
+
+test("should import stylesheets with same content", checkFixture, "same")
 
 test("should ignore & adjust external import", checkFixture, "ignore")
 
@@ -63,7 +86,7 @@ test(
   "charset-import"
 )
 
-test("should error if incompatable @charset statements", t => {
+test("should error if incompatible @charset statements", t => {
   t.plan(2)
   const file = "test/fixtures/charset-error.css"
   return postcss()
@@ -73,7 +96,7 @@ test("should error if incompatable @charset statements", t => {
       t.truthy(err)
       t.regex(
         err.message,
-        /Incompatable @charset statements:.+specified in.+specified in.+/s
+        /Incompatible @charset statements:.+specified in.+specified in.+/s
       )
     })
 })
@@ -130,6 +153,13 @@ test(
   [`${path.resolve("test/fixtures/imports/empty.css")} is empty`]
 )
 
+test(
+  "should be able to disable warnings for empty files",
+  checkFixture,
+  "empty-and-useless",
+  { path: "test/fixtures/imports", warnOnEmpty: false }
+)
+
 test("should work with no styles without throwing an error", t => {
   return postcss()
     .use(atImport())
diff --git a/test/layer.js b/test/layer.js
index 69d27af..3414ff0 100644
--- a/test/layer.js
+++ b/test/layer.js
@@ -49,7 +49,9 @@ test("should error when value is not a function", t => {
   return postcss()
     .use(atImport({ nameLayer: "not a function" }))
     .process("", { from: undefined })
-    .catch(error => t.is(error.message, "nameLayer option must be a function"))
+    .catch(error =>
+      t.regex(error.message, /nameLayer option must be a function/)
+    )
 })
 
 test("should throw when using anonymous layers without the nameLayer plugin option", t => {
diff --git a/test/plugins.js b/test/plugins.js
index 20cee88..31df1ff 100644
--- a/test/plugins.js
+++ b/test/plugins.js
@@ -35,7 +35,7 @@ test("should error when value is not an array", t => {
   return postcss()
     .use(atImport({ plugins: "foo" }))
     .process("", { from: undefined })
-    .catch(error => t.is(error.message, "plugins option must be an array"))
+    .catch(error => t.regex(error.message, /plugins option must be an array/))
 })
 
 test("should remain silent when value is an empty array", t => {