fix: don't duplicate import with same media in different case (#819)

webpack-contrib · Nov 28, 2018 · 9f66e33 · 9f66e33
1 parent 3ebdcd5
commit 9f66e33
Show file tree

Hide file tree

Showing 12 changed files with 88 additions and 51 deletions.
diff --git a/lib/loader.js b/lib/loader.js
@@ -51,7 +51,7 @@ module.exports = function loader(content, map) {
       const alreadyImported = {};
       const importJs = result.importItems
         .filter((imp) => {
-          if (!imp.mediaQuery) {
+          if (!imp.media) {
             if (alreadyImported[imp.url]) {
               return false;
             }
@@ -63,13 +63,13 @@ module.exports = function loader(content, map) {
           if (!loaderUtils.isUrlRequest(imp.url)) {
             return `exports.push([module.id, ${JSON.stringify(
               `@import url(${imp.url});`
-            )}, ${JSON.stringify(imp.mediaQuery)}]);`;
+            )}, ${JSON.stringify(imp.media)}]);`;
           }
           const importUrl = importUrlPrefix + imp.url;
           return `exports.i(require(${loaderUtils.stringifyRequest(
             this,
             importUrl
-          )}), ${JSON.stringify(imp.mediaQuery)});`;
+          )}), ${JSON.stringify(imp.media)});`;
         }, this)
         .join('\n');
 

diff --git a/lib/plugins/postcss-import-parser.js b/lib/plugins/postcss-import-parser.js
@@ -37,7 +37,10 @@ function parseImport(params) {
 
   return {
     url,
-    media: valueParser.stringify(nodes.slice(1)).trim(),
+    media: valueParser
+      .stringify(nodes.slice(1))
+      .trim()
+      .toLowerCase(),
   };
 }
 
@@ -71,15 +74,23 @@ module.exports = postcss.plugin(
           });
         }
 
+        atrule.remove();
+
+        const { media } = parsed;
         let { url } = parsed;
+        const isUrlRequest = loaderUtils.isUrlRequest(url);
 
-        if (loaderUtils.isUrlRequest(url)) {
+        if (isUrlRequest) {
           url = loaderUtils.urlToRequest(url);
         }
 
-        importItems.push({ url, mediaQuery: parsed.media });
+        const includes = importItems.find(
+          (el) => el.url === url && el.media === media
+        );
 
-        atrule.remove();
+        if (!includes) {
+          importItems.push({ url, media, isUrlRequest });
+        }
       });
 
       // eslint-disable-next-line no-param-reassign

diff --git a/lib/runtime/api.js b/lib/runtime/api.js
@@ -34,8 +34,8 @@ module.exports = function(useSourceMap) {
       var item = modules[i];
       // skip already imported module
       // this implementation is not 100% perfect for weird media query combinations
-      //  when a module is imported multiple times with different media queries.
-      //  I hope this will never occur (Hey this way we have smaller bundles)
+      // when a module is imported multiple times with different media queries.
+      // I hope this will never occur (Hey this way we have smaller bundles)
       if (item[0] == null || !alreadyImportedModules[item[0]]) {
         if (mediaQuery && !item[2]) {
           item[2] = mediaQuery;

diff --git a/test/__snapshots__/import-option.test.js.snap b/test/__snapshots__/import-option.test.js.snap
@@ -33,13 +33,16 @@ Array [
 @import url(test.css) screen and print;
 @import url(test.css) SCREEN AND PRINT;
 @import url(test.css)screen and print;
+@import url(test.css)  screen and print;
 @import url(test-media.css) screen and print;
 @import url(test-other.css) (min-width: 100px);
 @import url(http://example.com/style.css);
+@import url(http://example.com/style.css);
 @import url(http://example.com/style.css#hash);
 @import url(http://example.com/style.css?#hash);
 @import url(http://example.com/style.css?foo=bar#hash);
 @import url(http://example.com/other-style.css) screen and print;
+@import url(http://example.com/other-style.css) screen and print;
 @import url(\\"//example.com/style.css\\");
 @import url(~package/test.css);
 @import ;
@@ -48,6 +51,10 @@ Array [
 @import url('http://') :root {}
 @import url('query.css?foo=1&bar=1');
 @import url('other-query.css?foo=1&bar=1#hash');
+@import url('other-query.css?foo=1&bar=1#hash') screen and print;
+@import url('https://fonts.googleapis.com/css?family=Roboto');
+@import url('https://fonts.googleapis.com/css?family=Noto+Sans+TC');
+@import url('https://fonts.googleapis.com/css?family=Noto+Sans+TC|Roboto');
 
 .class {
   a: b c d;
@@ -68,7 +75,7 @@ exports[`import option false: module 1`] = `
 
 
 // module
-exports.push([module.id, \\"@import url(test.css);\\\\n@import url('test.css');\\\\n@import url(\\\\\\"test.css\\\\\\");\\\\n@IMPORT url(test.css);\\\\n@import URL(test.css);\\\\n@import url(test.css );\\\\n@import url( test.css);\\\\n@import url( test.css );\\\\n@import url(\\\\n  test.css\\\\n);\\\\n@import url();\\\\n@import url('');\\\\n@import url(\\\\\\"\\\\\\");\\\\n@import \\\\\\"test.css\\\\\\";\\\\n@import 'test.css';\\\\n@import '';\\\\n@import \\\\\\"\\\\\\";\\\\n@import \\\\\\"   \\\\\\";\\\\n@import \\\\\\"\\\\n\\\\\\";\\\\n@import url();\\\\n@import url('');\\\\n@import url(\\\\\\"\\\\\\");\\\\n@import url(test.css) screen and print;\\\\n@import url(test.css) SCREEN AND PRINT;\\\\n@import url(test.css)screen and print;\\\\n@import url(test-media.css) screen and print;\\\\n@import url(test-other.css) (min-width: 100px);\\\\n@import url(http://example.com/style.css);\\\\n@import url(http://example.com/style.css#hash);\\\\n@import url(http://example.com/style.css?#hash);\\\\n@import url(http://example.com/style.css?foo=bar#hash);\\\\n@import url(http://example.com/other-style.css) screen and print;\\\\n@import url(\\\\\\"//example.com/style.css\\\\\\");\\\\n@import url(~package/test.css);\\\\n@import ;\\\\n@import foo-bar;\\\\n@import-normalize;\\\\n@import url('http://') :root {}\\\\n@import url('query.css?foo=1&bar=1');\\\\n@import url('other-query.css?foo=1&bar=1#hash');\\\\n\\\\n.class {\\\\n  a: b c d;\\\\n}\\\\n\\\\n.foo {\\\\n  @import 'path.css';\\\\n}\\\\n\\", \\"\\"]);
+exports.push([module.id, \\"@import url(test.css);\\\\n@import url('test.css');\\\\n@import url(\\\\\\"test.css\\\\\\");\\\\n@IMPORT url(test.css);\\\\n@import URL(test.css);\\\\n@import url(test.css );\\\\n@import url( test.css);\\\\n@import url( test.css );\\\\n@import url(\\\\n  test.css\\\\n);\\\\n@import url();\\\\n@import url('');\\\\n@import url(\\\\\\"\\\\\\");\\\\n@import \\\\\\"test.css\\\\\\";\\\\n@import 'test.css';\\\\n@import '';\\\\n@import \\\\\\"\\\\\\";\\\\n@import \\\\\\"   \\\\\\";\\\\n@import \\\\\\"\\\\n\\\\\\";\\\\n@import url();\\\\n@import url('');\\\\n@import url(\\\\\\"\\\\\\");\\\\n@import url(test.css) screen and print;\\\\n@import url(test.css) SCREEN AND PRINT;\\\\n@import url(test.css)screen and print;\\\\n@import url(test.css)  screen and print;\\\\n@import url(test-media.css) screen and print;\\\\n@import url(test-other.css) (min-width: 100px);\\\\n@import url(http://example.com/style.css);\\\\n@import url(http://example.com/style.css);\\\\n@import url(http://example.com/style.css#hash);\\\\n@import url(http://example.com/style.css?#hash);\\\\n@import url(http://example.com/style.css?foo=bar#hash);\\\\n@import url(http://example.com/other-style.css) screen and print;\\\\n@import url(http://example.com/other-style.css) screen and print;\\\\n@import url(\\\\\\"//example.com/style.css\\\\\\");\\\\n@import url(~package/test.css);\\\\n@import ;\\\\n@import foo-bar;\\\\n@import-normalize;\\\\n@import url('http://') :root {}\\\\n@import url('query.css?foo=1&bar=1');\\\\n@import url('other-query.css?foo=1&bar=1#hash');\\\\n@import url('other-query.css?foo=1&bar=1#hash') screen and print;\\\\n@import url('https://fonts.googleapis.com/css?family=Roboto');\\\\n@import url('https://fonts.googleapis.com/css?family=Noto+Sans+TC');\\\\n@import url('https://fonts.googleapis.com/css?family=Noto+Sans+TC|Roboto');\\\\n\\\\n.class {\\\\n  a: b c d;\\\\n}\\\\n\\\\n.foo {\\\\n  @import 'path.css';\\\\n}\\\\n\\", \\"\\"]);
 
 // exports
 "
@@ -96,34 +103,18 @@ Array [
 ",
     "screen and print",
   ],
-  Array [
-    4,
-    ".test {
-  a: a;
-}
-",
-    "SCREEN AND PRINT",
-  ],
   Array [
     5,
-    ".test {
-  a: a;
-}
-",
-    "screen and print",
-  ],
-  Array [
-    7,
-    ".test {
-  a: b;
+    "a {
+  b: b;
 }
 ",
     "((min-width: 100px)) and (screen and print)",
   ],
   Array [
-    6,
+    4,
     ".test {
-  c: d;
+  c: c;
 }
 ",
     "screen and print",
@@ -159,29 +150,52 @@ Array [
     "",
   ],
   Array [
-    8,
+    6,
     ".test {
-  a: b
+  d: d
 }
 ",
     "",
   ],
   Array [
-    9,
+    7,
     ".query {
-  color: green;
+  e: e;
 }
 ",
     "",
   ],
   Array [
-    10,
+    8,
     ".other-query {
-  color: green;
+  f: f;
 }
 ",
     "",
   ],
+  Array [
+    9,
+    ".other-query {
+  f: f;
+}
+",
+    "screen and print",
+  ],
+  Array [
+    1,
+    "@import url(https://fonts.googleapis.com/css?family=Roboto);",
+    "",
+  ],
+  Array [
+    1,
+    "@import url(https://fonts.googleapis.com/css?family=Noto+Sans+TC);",
+    "",
+  ],
+  Array [
+    1,
+    "@import url(https://fonts.googleapis.com/css?family=Noto+Sans+TC|Roboto);",
+    "",
+  ],
   Array [
     1,
     "@import url();
@@ -218,8 +232,6 @@ exports[`import option true: module 1`] = `
 // imports
 exports.i(require(\\"-!../../../index.js??ref--4-0!./test.css\\"), \\"\\");
 exports.i(require(\\"-!../../../index.js??ref--4-0!./test.css\\"), \\"screen and print\\");
-exports.i(require(\\"-!../../../index.js??ref--4-0!./test.css\\"), \\"SCREEN AND PRINT\\");
-exports.i(require(\\"-!../../../index.js??ref--4-0!./test.css\\"), \\"screen and print\\");
 exports.i(require(\\"-!../../../index.js??ref--4-0!./test-media.css\\"), \\"screen and print\\");
 exports.i(require(\\"-!../../../index.js??ref--4-0!./test-other.css\\"), \\"(min-width: 100px)\\");
 exports.push([module.id, \\"@import url(http://example.com/style.css);\\", \\"\\"]);
@@ -231,6 +243,10 @@ exports.push([module.id, \\"@import url(//example.com/style.css);\\", \\"\\"]);
 exports.i(require(\\"-!../../../index.js??ref--4-0!package/test.css\\"), \\"\\");
 exports.i(require(\\"-!../../../index.js??ref--4-0!./query.css?foo=1&bar=1\\"), \\"\\");
 exports.i(require(\\"-!../../../index.js??ref--4-0!./other-query.css?foo=1&bar=1#hash\\"), \\"\\");
+exports.i(require(\\"-!../../../index.js??ref--4-0!./other-query.css?foo=1&bar=1#hash\\"), \\"screen and print\\");
+exports.push([module.id, \\"@import url(https://fonts.googleapis.com/css?family=Roboto);\\", \\"\\"]);
+exports.push([module.id, \\"@import url(https://fonts.googleapis.com/css?family=Noto+Sans+TC);\\", \\"\\"]);
+exports.push([module.id, \\"@import url(https://fonts.googleapis.com/css?family=Noto+Sans+TC|Roboto);\\", \\"\\"]);
 
 // module
 exports.push([module.id, \\"@import url();\\\\n@import url('');\\\\n@import url(\\\\\\"\\\\\\");\\\\n@import '';\\\\n@import \\\\\\"\\\\\\";\\\\n@import \\\\\\"   \\\\\\";\\\\n@import \\\\\\"\\\\n\\\\\\";\\\\n@import url();\\\\n@import url('');\\\\n@import url(\\\\\\"\\\\\\");\\\\n@import ;\\\\n@import foo-bar;\\\\n@import-normalize;\\\\n@import url('http://') :root {}\\\\n\\\\n.class {\\\\n  a: b c d;\\\\n}\\\\n\\\\n.foo {\\\\n  @import 'path.css';\\\\n}\\\\n\\", \\"\\"]);
@@ -285,14 +301,14 @@ Warning
   "ModuleWarning: Module Warning (from \`replaced original path\`):
 Warning
 
-(37:1) Unable to find uri in '@import '",
+(40:1) Unable to find uri in '@import '",
   "ModuleWarning: Module Warning (from \`replaced original path\`):
 Warning
 
-(38:1) Unable to find uri in '@import foo-bar'",
+(41:1) Unable to find uri in '@import foo-bar'",
   "ModuleWarning: Module Warning (from \`replaced original path\`):
 Warning
 
-(40:1) It looks like you didn't end your @import statement correctly. Child nodes are attached to it.",
+(43:1) It looks like you didn't end your @import statement correctly. Child nodes are attached to it.",
 ]
 `;
diff --git a/test/__snapshots__/loader.test.js.snap b/test/__snapshots__/loader.test.js.snap
@@ -37,8 +37,8 @@ module.exports = function(useSourceMap) {
       var item = modules[i];
       // skip already imported module
       // this implementation is not 100% perfect for weird media query combinations
-      //  when a module is imported multiple times with different media queries.
-      //  I hope this will never occur (Hey this way we have smaller bundles)
+      // when a module is imported multiple times with different media queries.
+      // I hope this will never occur (Hey this way we have smaller bundles)
       if (item[0] == null || !alreadyImportedModules[item[0]]) {
         if (mediaQuery && !item[2]) {
           item[2] = mediaQuery;
@@ -219,8 +219,8 @@ module.exports = function(useSourceMap) {
       var item = modules[i];
       // skip already imported module
       // this implementation is not 100% perfect for weird media query combinations
-      //  when a module is imported multiple times with different media queries.
-      //  I hope this will never occur (Hey this way we have smaller bundles)
+      // when a module is imported multiple times with different media queries.
+      // I hope this will never occur (Hey this way we have smaller bundles)
       if (item[0] == null || !alreadyImportedModules[item[0]]) {
         if (mediaQuery && !item[2]) {
           item[2] = mediaQuery;

diff --git a/test/fixtures/import/import.css b/test/fixtures/import/import.css
@@ -25,13 +25,16 @@
 @import url(test.css) screen and print;
 @import url(test.css) SCREEN AND PRINT;
 @import url(test.css)screen and print;
+@import url(test.css)  screen and print;
 @import url(test-media.css) screen and print;
 @import url(test-other.css) (min-width: 100px);
 @import url(http://example.com/style.css);
+@import url(http://example.com/style.css);
 @import url(http://example.com/style.css#hash);
 @import url(http://example.com/style.css?#hash);
 @import url(http://example.com/style.css?foo=bar#hash);
 @import url(http://example.com/other-style.css) screen and print;
+@import url(http://example.com/other-style.css) screen and print;
 @import url("//example.com/style.css");
 @import url(~package/test.css);
 @import ;
@@ -40,6 +43,10 @@
 @import url('http://') :root {}
 @import url('query.css?foo=1&bar=1');
 @import url('other-query.css?foo=1&bar=1#hash');
+@import url('other-query.css?foo=1&bar=1#hash') screen and print;
+@import url('https://fonts.googleapis.com/css?family=Roboto');
+@import url('https://fonts.googleapis.com/css?family=Noto+Sans+TC');
+@import url('https://fonts.googleapis.com/css?family=Noto+Sans+TC|Roboto');
 
 .class {
   a: b c d;

diff --git a/test/fixtures/import/node_modules/package/test.css b/test/fixtures/import/node_modules/package/test.css
diff --git a/test/fixtures/import/other-query.css b/test/fixtures/import/other-query.css
@@ -1,3 +1,3 @@
 .other-query {
-  color: green;
+  f: f;
 }
diff --git a/test/fixtures/import/query.css b/test/fixtures/import/query.css
@@ -1,3 +1,3 @@
 .query {
-  color: green;
+  e: e;
 }
diff --git a/test/fixtures/import/test-media.css b/test/fixtures/import/test-media.css
@@ -1,5 +1,5 @@
-@import url('test-other.css') (min-width: 100px);
+@import url('test-nested-media.css') (min-width: 100px);
 
 .test {
-  c: d;
+  c: c;
 }
diff --git a/test/fixtures/import/test-nested-media.css b/test/fixtures/import/test-nested-media.css
@@ -0,0 +1,3 @@
+a {
+  b: b;
+}
diff --git a/test/fixtures/import/test-other.css b/test/fixtures/import/test-other.css
@@ -1,3 +1,3 @@
 .test {
-  a: b;
+  d: d;
 }