Repository: tivac/modular-css
Pull request #577 (2019-04-02 23:48:34)

fix(rollup-rewriter): include static dependencies
<!--- Provide a general summary of your changes in the Title above -->

## Description
<!--- Describe your changes in detail -->
At dayjob :tm: a coworker ran into an issue where a dynamically loaded chunk that was being rewritten to include its CSS deps some of those deps were missing. Turns out that `@modular-css/rollup-rewriter` didn't include the CSS required by any static `imports` contained within the dynamic import chunk entry point.

So now it does.

## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
Dynamic chunk entrypoints that have static dependencies that include CSS should be properly loaded.

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->
Added specimens & a test. Verified it failed before this change, then passes afterwards.

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
- [x] My code follows the code style of this project.
- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I have read the **CONTRIBUTING** document.
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.


codecov[bot]: # [Codecov](https://codecov.io/gh/tivac/modular-css/pull/577?src=pr&el=h1) Report
> Merging [#577](https://codecov.io/gh/tivac/modular-css/pull/577?src=pr&el=desc) into [master](https://codecov.io/gh/tivac/modular-css/commit/ec746a9fd5398aa7c1396aa0876bc0d342af9cdb?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/tivac/modular-css/pull/577/graphs/tree.svg?width=650&token=EIUJKcuTE8&height=150&src=pr)](https://codecov.io/gh/tivac/modular-css/pull/577?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master     #577   +/-   ##
=======================================
  Coverage   98.89%   98.89%           
=======================================
  Files          47       47           
  Lines        1178     1178           
  Branches      183      183           
=======================================
  Hits         1165     1165           
  Misses         13       13
```


| [Impacted Files](https://codecov.io/gh/tivac/modular-css/pull/577?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [packages/rollup-rewriter/rewriter.js](https://codecov.io/gh/tivac/modular-css/pull/577/diff?src=pr&el=tree#diff-cGFja2FnZXMvcm9sbHVwLXJld3JpdGVyL3Jld3JpdGVyLmpz) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/tivac/modular-css/pull/577?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/tivac/modular-css/pull/577?src=pr&el=footer). Last update [ec746a9...b5290de](https://codecov.io/gh/tivac/modular-css/pull/577?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

codecov[bot]: # [Codecov](https://codecov.io/gh/tivac/modular-css/pull/577?src=pr&el=h1) Report
> Merging [#577](https://codecov.io/gh/tivac/modular-css/pull/577?src=pr&el=desc) into [master](https://codecov.io/gh/tivac/modular-css/commit/ec746a9fd5398aa7c1396aa0876bc0d342af9cdb?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/tivac/modular-css/pull/577/graphs/tree.svg?width=650&token=EIUJKcuTE8&height=150&src=pr)](https://codecov.io/gh/tivac/modular-css/pull/577?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master     #577   +/-   ##
=======================================
  Coverage   98.89%   98.89%           
=======================================
  Files          47       47           
  Lines        1178     1178           
  Branches      183      183           
=======================================
  Hits         1165     1165           
  Misses         13       13
```


| [Impacted Files](https://codecov.io/gh/tivac/modular-css/pull/577?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [packages/rollup-rewriter/rewriter.js](https://codecov.io/gh/tivac/modular-css/pull/577/diff?src=pr&el=tree#diff-cGFja2FnZXMvcm9sbHVwLXJld3JpdGVyL3Jld3JpdGVyLmpz) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/tivac/modular-css/pull/577?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/tivac/modular-css/pull/577?src=pr&el=footer). Last update [ec746a9...b5290de](https://codecov.io/gh/tivac/modular-css/pull/577?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

TravisBuddy: Hey @tivac,
Something went wrong with the build.

TravisCI finished with status `errored`, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

<a href="https:&#x2F;&#x2F;travis-ci.org&#x2F;tivac&#x2F;modular-css&#x2F;builds&#x2F;515021631">View build log</a>

###### TravisBuddy Request Identifier: c4d7a230-55d2-11e9-bc42-792bcead230c


Patch:
diff --git a/packages/rollup-rewriter/rewriter.js b/packages/rollup-rewriter/rewriter.js
index 191ad1eb..6bc17e5d 100644
--- a/packages/rollup-rewriter/rewriter.js
+++ b/packages/rollup-rewriter/rewriter.js
@@ -57,13 +57,13 @@ module.exports = (opts) => {
                 }
 
                 graph.addNode(entry);
-                
+
                 imported.forEach((file) => {
                     graph.addNode(file);
                     graph.addDependency(entry, file);
                 });
             });
-            
+
             entries.forEach((deps, entry) => {
                 const { code } = chunks[entry];
 
@@ -88,7 +88,11 @@ module.exports = (opts) => {
                     const { index } = result;
 
                     // eslint-disable-next-line no-loop-func
-                    const css = [ ...graph.dependenciesOf(file), file ].reduce((out, curr) => {
+                    const css = [
+                        ...graph.dependenciesOf(file),
+                        ...(file in chunks ? chunks[file].imports : []),
+                        file,
+                    ].reduce((out, curr) => {
                         const { assets = [] } = chunks[curr];
 
                         assets.forEach((asset) => out.add(asset));


Test patch:
diff --git a/packages/rollup-rewriter/test/__snapshots__/rewriter.test.js.snap b/packages/rollup-rewriter/test/__snapshots__/rewriter.test.js.snap
index 8a9a2dbe..571cecbd 100644
--- a/packages/rollup-rewriter/test/__snapshots__/rewriter.test.js.snap
+++ b/packages/rollup-rewriter/test/__snapshots__/rewriter.test.js.snap
@@ -2,6 +2,367 @@
 
 exports[`rollup-rewriter should error on unsupported formats ("cjs") 1`] = `"Unsupported format: cjs. Supported formats are [\\"amd\\",\\"es\\",\\"esm\\",\\"system\\"]"`;
 
+exports[`rollup-rewriter should include css for static imports used by a dynamic import ("amd") 1`] = `
+Object {
+  "assets/dynamic1.css": "
+/* packages/rollup-rewriter/test/specimens/dynamic-shared-imports/dynamic1.css */
+.dynamic1 {
+    color: dynamic1;
+}
+",
+  "assets/entry1.css": "
+/* packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry1.css */
+.entry1 {
+    color: entry1;
+}
+",
+  "assets/entry2.css": "
+/* packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry2.css */
+.entry2 {
+    color: entry2;
+}
+",
+  "assets/static1.css": "
+/* packages/rollup-rewriter/test/specimens/dynamic-shared-imports/static1.css */
+.static1 {
+    color: static1;
+}
+",
+  "chunk.js": "
+define(['exports'], function (exports) { 'use strict';
+
+    var css = {
+        \\"static1\\": \\"static1\\"
+    };
+
+    console.log(css);
+
+    var static1 = \\"static1.js\\";
+
+    exports.static1 = static1;
+
+});
+",
+  "chunk2.js": "
+define(['exports', './chunk.js'], function (exports, __chunk_1) { 'use strict';
+
+    var css = {
+        \\"dynamic1\\": \\"dynamic1\\"
+    };
+
+    console.log(__chunk_1.static1, css);
+
+    var dynamic1 = \\"dynamic1.js\\";
+
+    exports.default = dynamic1;
+
+});
+",
+  "entry1.js": "
+define(['./chunk.js'], function (__chunk_1) { 'use strict';
+
+    var css = {
+        \\"entry1\\": \\"entry1\\"
+    };
+
+    console.log(__chunk_1.static1, css);
+
+});
+",
+  "entry2.js": "
+define(['require'], function (require) { 'use strict';
+
+    var css = {
+        \\"entry2\\": \\"entry2\\"
+    };
+
+    console.log(css);
+
+    (function() {
+        new Promise(function (resolve, reject) { Promise.all([
+    lazyload(\\"./assets/static1.css\\"),
+lazyload(\\"./assets/dynamic1.css\\"),
+    new Promise(function (resolve, reject) { require(['./chunk2.js'], resolve, reject) })
+])
+.then((results) => resolve(results[results.length - 1]))
+.catch(reject) }).then(console.log);
+    }());
+
+});
+",
+}
+`;
+
+exports[`rollup-rewriter should include css for static imports used by a dynamic import ("es") 1`] = `
+Object {
+  "assets/dynamic1.css": "
+/* packages/rollup-rewriter/test/specimens/dynamic-shared-imports/dynamic1.css */
+.dynamic1 {
+    color: dynamic1;
+}
+",
+  "assets/entry1.css": "
+/* packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry1.css */
+.entry1 {
+    color: entry1;
+}
+",
+  "assets/entry2.css": "
+/* packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry2.css */
+.entry2 {
+    color: entry2;
+}
+",
+  "assets/static1.css": "
+/* packages/rollup-rewriter/test/specimens/dynamic-shared-imports/static1.css */
+.static1 {
+    color: static1;
+}
+",
+  "chunk.js": "
+var css = {
+    \\"static1\\": \\"static1\\"
+};
+
+console.log(css);
+
+var static1 = \\"static1.js\\";
+
+export { static1 as a };
+",
+  "chunk2.js": "
+import { a as static1 } from './chunk.js';
+
+var css = {
+    \\"dynamic1\\": \\"dynamic1\\"
+};
+
+console.log(static1, css);
+
+var dynamic1 = \\"dynamic1.js\\";
+
+export default dynamic1;
+",
+  "entry1.js": "
+import { a as static1 } from './chunk.js';
+
+var css = {
+    \\"entry1\\": \\"entry1\\"
+};
+
+console.log(static1, css);
+",
+  "entry2.js": "
+var css = {
+    \\"entry2\\": \\"entry2\\"
+};
+
+console.log(css);
+
+(function() {
+    Promise.all([
+    lazyload(\\"./assets/static1.css\\"),
+lazyload(\\"./assets/dynamic1.css\\"),
+    import('./chunk2.js')
+])
+.then((results) => results[results.length - 1]).then(console.log);
+}());
+",
+}
+`;
+
+exports[`rollup-rewriter should include css for static imports used by a dynamic import ("esm") 1`] = `
+Object {
+  "assets/dynamic1.css": "
+/* packages/rollup-rewriter/test/specimens/dynamic-shared-imports/dynamic1.css */
+.dynamic1 {
+    color: dynamic1;
+}
+",
+  "assets/entry1.css": "
+/* packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry1.css */
+.entry1 {
+    color: entry1;
+}
+",
+  "assets/entry2.css": "
+/* packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry2.css */
+.entry2 {
+    color: entry2;
+}
+",
+  "assets/static1.css": "
+/* packages/rollup-rewriter/test/specimens/dynamic-shared-imports/static1.css */
+.static1 {
+    color: static1;
+}
+",
+  "chunk.js": "
+var css = {
+    \\"static1\\": \\"static1\\"
+};
+
+console.log(css);
+
+var static1 = \\"static1.js\\";
+
+export { static1 as a };
+",
+  "chunk2.js": "
+import { a as static1 } from './chunk.js';
+
+var css = {
+    \\"dynamic1\\": \\"dynamic1\\"
+};
+
+console.log(static1, css);
+
+var dynamic1 = \\"dynamic1.js\\";
+
+export default dynamic1;
+",
+  "entry1.js": "
+import { a as static1 } from './chunk.js';
+
+var css = {
+    \\"entry1\\": \\"entry1\\"
+};
+
+console.log(static1, css);
+",
+  "entry2.js": "
+var css = {
+    \\"entry2\\": \\"entry2\\"
+};
+
+console.log(css);
+
+(function() {
+    Promise.all([
+    lazyload(\\"./assets/static1.css\\"),
+lazyload(\\"./assets/dynamic1.css\\"),
+    import('./chunk2.js')
+])
+.then((results) => results[results.length - 1]).then(console.log);
+}());
+",
+}
+`;
+
+exports[`rollup-rewriter should include css for static imports used by a dynamic import ("system") 1`] = `
+Object {
+  "assets/dynamic1.css": "
+/* packages/rollup-rewriter/test/specimens/dynamic-shared-imports/dynamic1.css */
+.dynamic1 {
+    color: dynamic1;
+}
+",
+  "assets/entry1.css": "
+/* packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry1.css */
+.entry1 {
+    color: entry1;
+}
+",
+  "assets/entry2.css": "
+/* packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry2.css */
+.entry2 {
+    color: entry2;
+}
+",
+  "assets/static1.css": "
+/* packages/rollup-rewriter/test/specimens/dynamic-shared-imports/static1.css */
+.static1 {
+    color: static1;
+}
+",
+  "chunk.js": "
+System.register([], function (exports, module) {
+    'use strict';
+    return {
+        execute: function () {
+
+            var css = {
+                \\"static1\\": \\"static1\\"
+            };
+
+            console.log(css);
+
+            var static1 = exports('a', \\"static1.js\\");
+
+        }
+    };
+});
+",
+  "chunk2.js": "
+System.register(['./chunk.js'], function (exports, module) {
+    'use strict';
+    var static1;
+    return {
+        setters: [function (module) {
+            static1 = module.a;
+        }],
+        execute: function () {
+
+            var css = {
+                \\"dynamic1\\": \\"dynamic1\\"
+            };
+
+            console.log(static1, css);
+
+            var dynamic1 = exports('default', \\"dynamic1.js\\");
+
+        }
+    };
+});
+",
+  "entry1.js": "
+System.register(['./chunk.js'], function (exports, module) {
+    'use strict';
+    var static1;
+    return {
+        setters: [function (module) {
+            static1 = module.a;
+        }],
+        execute: function () {
+
+            var css = {
+                \\"entry1\\": \\"entry1\\"
+            };
+
+            console.log(static1, css);
+
+        }
+    };
+});
+",
+  "entry2.js": "
+System.register([], function (exports, module) {
+    'use strict';
+    return {
+        execute: function () {
+
+            var css = {
+                \\"entry2\\": \\"entry2\\"
+            };
+
+            console.log(css);
+
+            (function() {
+                Promise.all([
+    lazyload(\\"./assets/static1.css\\"),
+lazyload(\\"./assets/dynamic1.css\\"),
+    module.import('./chunk2.js')
+])
+.then((results) => results[results.length - 1]).then(console.log);
+            }());
+
+        }
+    };
+});
+",
+}
+`;
+
 exports[`rollup-rewriter should log details in verbose mode 1`] = `
 Array [
   Array [
diff --git a/packages/rollup-rewriter/test/rewriter.test.js b/packages/rollup-rewriter/test/rewriter.test.js
index 6c8a18d8..6274097a 100644
--- a/packages/rollup-rewriter/test/rewriter.test.js
+++ b/packages/rollup-rewriter/test/rewriter.test.js
@@ -140,6 +140,34 @@ describe("rollup-rewriter", () => {
         expect(result).toMatchRollupSnapshot();
     });
 
+    it.each(formats)("should include css for static imports used by a dynamic import (%p)", async (format) => {
+        const bundle = await rollup({
+            input : [
+                require.resolve("./specimens/dynamic-shared-imports/entry1.js"),
+                require.resolve("./specimens/dynamic-shared-imports/entry2.js"),
+            ],
+            plugins : [
+                css({
+                    namer,
+                    map,
+                }),
+                rewriter({
+                    loadfn : "lazyload",
+                }),
+            ],
+        });
+
+        const result = await bundle.generate({
+            format,
+            sourcemap,
+
+            assetFileNames,
+            chunkFileNames,
+        });
+
+        expect(result).toMatchRollupSnapshot();
+    });
+
     it("should log details in verbose mode", async () => {
         const { logSnapshot } = logs();
 
diff --git a/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/dynamic1.css b/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/dynamic1.css
new file mode 100644
index 00000000..c110fad8
--- /dev/null
+++ b/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/dynamic1.css
@@ -0,0 +1,3 @@
+.dynamic1 {
+    color: dynamic1;
+}
diff --git a/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/dynamic1.js b/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/dynamic1.js
new file mode 100644
index 00000000..faab847b
--- /dev/null
+++ b/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/dynamic1.js
@@ -0,0 +1,7 @@
+import static1 from "./static1.js";
+
+import css from "./dynamic1.css";
+
+console.log(static1, css);
+
+export default "dynamic1.js";
diff --git a/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry1.css b/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry1.css
new file mode 100644
index 00000000..e12c7a66
--- /dev/null
+++ b/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry1.css
@@ -0,0 +1,3 @@
+.entry1 {
+    color: entry1;
+}
diff --git a/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry1.js b/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry1.js
new file mode 100644
index 00000000..135eecfd
--- /dev/null
+++ b/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry1.js
@@ -0,0 +1,5 @@
+import static1 from "./static1.js";
+
+import css from "./entry1.css";
+
+console.log(static1, css);
diff --git a/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry2.css b/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry2.css
new file mode 100644
index 00000000..76dbbbff
--- /dev/null
+++ b/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry2.css
@@ -0,0 +1,3 @@
+.entry2 {
+    color: entry2;
+}
diff --git a/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry2.js b/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry2.js
new file mode 100644
index 00000000..bf9ab623
--- /dev/null
+++ b/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry2.js
@@ -0,0 +1,7 @@
+import css from "./entry2.css";
+
+console.log(css);
+
+(function() {
+    import("./dynamic1.js").then(console.log);
+}());
diff --git a/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/static1.css b/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/static1.css
new file mode 100644
index 00000000..bb180cbc
--- /dev/null
+++ b/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/static1.css
@@ -0,0 +1,3 @@
+.static1 {
+    color: static1;
+}
diff --git a/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/static1.js b/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/static1.js
new file mode 100644
index 00000000..a48e967a
--- /dev/null
+++ b/packages/rollup-rewriter/test/specimens/dynamic-shared-imports/static1.js
@@ -0,0 +1,5 @@
+import css from "./static1.css";
+
+console.log(css);
+
+export default "static1.js";