Repository: tivac/modular-css (js)

Issue:
rollup-rewriter doesn't handle unknown deps well
<!--- Provide a general summary of the issue in the Title above -->

## Expected Behavior
When dealing with unknown dependencies, `@modular-css/rollup-rewriter` should cleanly handle them without exploding.

## Current Behavior
<!--- Tell us what happens instead of the expected behavior -->
🔥

```
(!) Unresolved dependencies
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
@sveltejs/svelte-virtual-list (imported by thing.svelte)
[!] (plugin @modular-css/rollup-rewriter) TypeError: Cannot destructure property `assets` of 'undefined' or 'null'.
TypeError: Cannot destructure property `assets` of 'undefined' or 'null'.
    at css.reduce (c:\project\node_modules\@modular-css\rollup-rewriter\rewriter.js:100:55)
    at Array.reduce (<anonymous>)
    at entries.forEach (c:\project\node_modules\@modular-css\rollup-rewriter\rewriter.js:99:23)
    at Map.forEach (<anonymous>)
    at Object.generateBundle (c:\project\node_modules\@modular-css\rollup-rewriter\rewriter.js:68:21)
    at Promise.resolve.then (c:\project\node_modules\rollup\dist\rollup.js:16621:25)
```

Error is coming from this chunk of code

https://github.com/tivac/modular-css/blob/0e913b002823711ee34017e60c4f5c323b190782/packages/rollup-rewriter/rewriter.js#L103-L109

## Possible Solution
<!--- Not obligatory, but suggest a fix/reason for the bug, -->
A conditional early-out before the destructuring would solve that.

```diff
const css = [
    ...graph.dependenciesOf(file),
    ...(file in chunks ? chunks[file].imports : []),
    file,
].reduce((out, curr) => {
+   if(!chunks[curr]) {
+       return out;
+   }
+
    const { assets = [] } = chunks[curr];

    assets.forEach((asset) => out.add(asset));

    return out;
}, new Set());
```

Pull request:
fix(rewriter): don't explode on external chunks
<!--- Provide a general summary of your changes in the Title above -->

## Description
<!--- Describe your changes in detail -->
Check to make sure a chunk is actually a thing before trying to read assets off of it.

## 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. -->

## 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. -->
Fixes #688

## 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.


Patch:
diff --git a/packages/rollup-rewriter/rewriter.js b/packages/rollup-rewriter/rewriter.js
index 66d14498..1072c499 100644
--- a/packages/rollup-rewriter/rewriter.js
+++ b/packages/rollup-rewriter/rewriter.js
@@ -107,6 +107,10 @@ module.exports = (opts = {}) => {
                         ...(file in chunks ? chunks[file].imports : []),
                         file,
                     ].reduce((out, curr) => {
+                        if(!chunks[curr]) {
+                            return out;
+                        }
+
                         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 ae6a96e4..857a68de 100644
--- a/packages/rollup-rewriter/test/__snapshots__/rewriter.test.js.snap
+++ b/packages/rollup-rewriter/test/__snapshots__/rewriter.test.js.snap
@@ -2,6 +2,206 @@
 
 exports[`rollup-rewriter should error on unsupported formats ("cjs") 1`] = `"Unsupported format: cjs. Supported formats are [\\"amd\\",\\"es\\",\\"esm\\",\\"system\\"]"`;
 
+exports[`rollup-rewriter should ignore unknown imports 1`] = `
+Object {
+  "a.js": "
+define(['require', 'external'], function (require, external) { 'use strict';
+
+    external = external && Object.prototype.hasOwnProperty.call(external, 'default') ? external['default'] : external;
+
+    var css = {
+        \\"a\\": \\"a\\"
+    };
+
+    function a() {
+        console.log(css, external);
+
+        new Promise(function (resolve, reject) { Promise.all([
+    lazyload(\\"./assets/c.css\\"),
+    new Promise(function (resolve, reject) { require(['./c'], resolve, reject) })
+])
+.then((results) => resolve(results[results.length - 1]))
+.catch(reject) }).then(console.log);
+    }
+
+    return a;
+
+});
+",
+  "assets/a.css": "
+/* packages/rollup-rewriter/test/specimens/external-import/a.css */
+.a { color: aqua; }
+",
+  "assets/c.css": "
+/* packages/rollup-rewriter/test/specimens/external-import/c.css */
+.c {
+    color: cyan;
+}
+",
+  "c.js": "
+define(['exports'], function (exports) { 'use strict';
+
+    var css = {
+        \\"c\\": \\"c\\"
+    };
+
+    console.log(css);
+
+    exports.default = css;
+
+});
+",
+}
+`;
+
+exports[`rollup-rewriter should ignore unknown imports 2`] = `
+Object {
+  "a.js": "
+import external from 'external';
+
+var css = {
+    \\"a\\": \\"a\\"
+};
+
+function a() {
+    console.log(css, external);
+
+    Promise.all([
+    lazyload(\\"./assets/c.css\\"),
+    import('./c.js')
+])
+.then((results) => results[results.length - 1]).then(console.log);
+}
+
+export default a;
+",
+  "assets/a.css": "
+/* packages/rollup-rewriter/test/specimens/external-import/a.css */
+.a { color: aqua; }
+",
+  "assets/c.css": "
+/* packages/rollup-rewriter/test/specimens/external-import/c.css */
+.c {
+    color: cyan;
+}
+",
+  "c.js": "
+var css = {
+    \\"c\\": \\"c\\"
+};
+
+console.log(css);
+
+export default css;
+",
+}
+`;
+
+exports[`rollup-rewriter should ignore unknown imports 3`] = `
+Object {
+  "a.js": "
+import external from 'external';
+
+var css = {
+    \\"a\\": \\"a\\"
+};
+
+function a() {
+    console.log(css, external);
+
+    Promise.all([
+    lazyload(\\"./assets/c.css\\"),
+    import('./c.js')
+])
+.then((results) => results[results.length - 1]).then(console.log);
+}
+
+export default a;
+",
+  "assets/a.css": "
+/* packages/rollup-rewriter/test/specimens/external-import/a.css */
+.a { color: aqua; }
+",
+  "assets/c.css": "
+/* packages/rollup-rewriter/test/specimens/external-import/c.css */
+.c {
+    color: cyan;
+}
+",
+  "c.js": "
+var css = {
+    \\"c\\": \\"c\\"
+};
+
+console.log(css);
+
+export default css;
+",
+}
+`;
+
+exports[`rollup-rewriter should ignore unknown imports 4`] = `
+Object {
+  "a.js": "
+System.register(['external'], function (exports, module) {
+    'use strict';
+    var external;
+    return {
+        setters: [function (module) {
+            external = module.default;
+        }],
+        execute: function () {
+
+            exports('default', a);
+
+            var css = {
+                \\"a\\": \\"a\\"
+            };
+
+            function a() {
+                console.log(css, external);
+
+                Promise.all([
+    lazyload(\\"./assets/c.css\\"),
+    module.import('./c.js')
+])
+.then((results) => results[results.length - 1]).then(console.log);
+            }
+
+        }
+    };
+});
+",
+  "assets/a.css": "
+/* packages/rollup-rewriter/test/specimens/external-import/a.css */
+.a { color: aqua; }
+",
+  "assets/c.css": "
+/* packages/rollup-rewriter/test/specimens/external-import/c.css */
+.c {
+    color: cyan;
+}
+",
+  "c.js": "
+System.register([], function (exports) {
+    'use strict';
+    return {
+        execute: function () {
+
+            var css = {
+                \\"c\\": \\"c\\"
+            };
+
+            console.log(css);
+            exports('default', css);
+
+        }
+    };
+});
+",
+}
+`;
+
 exports[`rollup-rewriter should include css for static imports used by a dynamic import ("amd") 1`] = `
 Object {
   "assets/dynamic1.css": "
diff --git a/packages/rollup-rewriter/test/rewriter.test.js b/packages/rollup-rewriter/test/rewriter.test.js
index c8e16b7d..c8060001 100644
--- a/packages/rollup-rewriter/test/rewriter.test.js
+++ b/packages/rollup-rewriter/test/rewriter.test.js
@@ -18,7 +18,7 @@ const chunkFileNames = "[name].js";
 const map = false;
 const sourcemap = false;
 
-const formats = [[ "amd" ], [ "es" ], [ "esm" ], [ "system" ]];
+const formats = [ "amd", "es", "esm", "system" ];
 
 describe("rollup-rewriter", () => {
     beforeAll(() => shell.rm("-rf", prefix("./output/*")));
@@ -28,7 +28,7 @@ describe("rollup-rewriter", () => {
     });
 
     it.each([
-        [ "cjs" ],
+        "cjs",
     ])("should error on unsupported formats (%p)", async (format) => {
         const bundle = await rollup({
             input : [
@@ -169,6 +169,32 @@ describe("rollup-rewriter", () => {
         expect(result).toMatchRollupSnapshot();
     });
 
+    it.each(formats)("should ignore unknown imports", async (format) => {
+        const bundle = await rollup({
+            input    : require.resolve("./specimens/external-import/a.js"),
+            external : [ "external" ],
+            plugins  : [
+                css({
+                    namer,
+                    map,
+                }),
+                rewriter({
+                    loadfn : "lazyload",
+                }),
+            ],
+        });
+
+        const result = await bundle.generate({
+            format,
+            sourcemap,
+
+            assetFileNames,
+            chunkFileNames,
+        });
+
+        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 : [
@@ -197,6 +223,7 @@ describe("rollup-rewriter", () => {
         expect(result).toMatchRollupSnapshot();
     });
 
+    // eslint-disable-next-line jest/expect-expect
     it("should log details in verbose mode", async () => {
         const { logSnapshot } = logs();
 
diff --git a/packages/rollup-rewriter/test/specimens/external-import/a.css b/packages/rollup-rewriter/test/specimens/external-import/a.css
new file mode 100644
index 00000000..eb93c954
--- /dev/null
+++ b/packages/rollup-rewriter/test/specimens/external-import/a.css
@@ -0,0 +1,1 @@
+.a { color: aqua; }
diff --git a/packages/rollup-rewriter/test/specimens/external-import/a.js b/packages/rollup-rewriter/test/specimens/external-import/a.js
new file mode 100644
index 00000000..a14b61b3
--- /dev/null
+++ b/packages/rollup-rewriter/test/specimens/external-import/a.js
@@ -0,0 +1,8 @@
+import external from "external";
+import css from "./a.css";
+
+export default function() {
+    console.log(css, external);
+
+    import("./c.js").then(console.log);
+}
diff --git a/packages/rollup-rewriter/test/specimens/external-import/c.css b/packages/rollup-rewriter/test/specimens/external-import/c.css
new file mode 100644
index 00000000..6ffbc835
--- /dev/null
+++ b/packages/rollup-rewriter/test/specimens/external-import/c.css
@@ -0,0 +1,3 @@
+.c {
+    color: cyan;
+}
diff --git a/packages/rollup-rewriter/test/specimens/external-import/c.js b/packages/rollup-rewriter/test/specimens/external-import/c.js
new file mode 100644
index 00000000..13c3c44d
--- /dev/null
+++ b/packages/rollup-rewriter/test/specimens/external-import/c.js
@@ -0,0 +1,5 @@
+import css from "./c.css";
+
+console.log(css);
+
+export default css;