From ca499c61d8feece6789ff788e6a6b17bd92d0cd4 Mon Sep 17 00:00:00 2001 From: Pat Cavit Date: Tue, 2 Apr 2019 22:48:19 -0700 Subject: [PATCH] fix(rollup-rewriter): include static dependencies (#577) * test: add failing test for rewriter bug It's not including dependencies of any static imports for the dynamic modules being found, which means that depending on your flow you can end up with incomplete styling. * fix: include static imports of dynamic deps So every CSS file is included before the dynamic chunk is loaded --- packages/rollup-rewriter/rewriter.js | 10 +- .../test/__snapshots__/rewriter.test.js.snap | 361 ++++++++++++++++++ .../rollup-rewriter/test/rewriter.test.js | 28 ++ .../dynamic-shared-imports/dynamic1.css | 3 + .../dynamic-shared-imports/dynamic1.js | 7 + .../dynamic-shared-imports/entry1.css | 3 + .../dynamic-shared-imports/entry1.js | 5 + .../dynamic-shared-imports/entry2.css | 3 + .../dynamic-shared-imports/entry2.js | 7 + .../dynamic-shared-imports/static1.css | 3 + .../dynamic-shared-imports/static1.js | 5 + 11 files changed, 432 insertions(+), 3 deletions(-) create mode 100644 packages/rollup-rewriter/test/specimens/dynamic-shared-imports/dynamic1.css create mode 100644 packages/rollup-rewriter/test/specimens/dynamic-shared-imports/dynamic1.js create mode 100644 packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry1.css create mode 100644 packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry1.js create mode 100644 packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry2.css create mode 100644 packages/rollup-rewriter/test/specimens/dynamic-shared-imports/entry2.js create mode 100644 packages/rollup-rewriter/test/specimens/dynamic-shared-imports/static1.css create mode 100644 packages/rollup-rewriter/test/specimens/dynamic-shared-imports/static1.js diff --git a/packages/rollup-rewriter/rewriter.js b/packages/rollup-rewriter/rewriter.js index 191ad1eb3..6bc17e5d1 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)); diff --git a/packages/rollup-rewriter/test/__snapshots__/rewriter.test.js.snap b/packages/rollup-rewriter/test/__snapshots__/rewriter.test.js.snap index 8a9a2dbea..571cecbd4 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 6c8a18d8f..6274097ad 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 000000000..c110fad84 --- /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 000000000..faab847b0 --- /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 000000000..e12c7a662 --- /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 000000000..135eecfde --- /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 000000000..76dbbbff9 --- /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 000000000..bf9ab6239 --- /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 000000000..bb180cbc8 --- /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 000000000..a48e967af --- /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";