From 5515b3369a93d5340ea9be328e24267a08b39863 Mon Sep 17 00:00:00 2001 From: Pat Cavit Date: Mon, 21 Jan 2019 12:02:32 -0800 Subject: [PATCH 1/2] fix: only rewrite import() when necessary If the chunk doesn't have any assets there's no point trying to rewrite it and exploding. --- packages/rollup-rewriter/rewriter.js | 44 +++++++++++++++------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/packages/rollup-rewriter/rewriter.js b/packages/rollup-rewriter/rewriter.js index 62cd4283e..25322c594 100644 --- a/packages/rollup-rewriter/rewriter.js +++ b/packages/rollup-rewriter/rewriter.js @@ -8,7 +8,7 @@ const formats = { es : require("./formats/es.js"), amd : require("./formats/amd.js"), system : require("./formats/system.js"), - + // Just an alias... esm : require("./formats/es.js"), }; @@ -45,10 +45,10 @@ module.exports = (opts) => { code = "", dynamicImports = [], } = chunk; - + // Guard against https://github.com/rollup/rollup/issues/2659 const deps = dynamicImports.filter(Boolean); - + if(isAsset || !deps.length || !assets.length) { return; } @@ -56,38 +56,40 @@ module.exports = (opts) => { const { regex, loader, load } = formats[format]; const search = regex(deps.map(escape).join("|")); - + const str = new MagicString(code); - + if(options.loader) { loader(options, str); } - + // Yay stateful regexes search.lastIndex = 0; - + let result = search.exec(code); - + while(result) { // Pull useful values out of the regex result const [ statement, file ] = result; const { index } = result; - - const imports = chunks[file].assets.map((dep) => - `${options.loadfn}("./${dep}")` - ); - - str.overwrite( - index, - index + statement.length, - dedent(load(options, imports.join(",\n"), statement)) - ); - + + if(chunks[file].assets) { + const imports = chunks[file].assets.map((dep) => + `${options.loadfn}("./${dep}")` + ); + + str.overwrite( + index, + index + statement.length, + dedent(load(options, imports.join(",\n"), statement)) + ); + } + result = search.exec(code); } - + log("Updating", entry); - + chunk.code = str.toString(); }); }, From a18ce004a19fc1f97119cb90b494c73c97979402 Mon Sep 17 00:00:00 2001 From: Pat Cavit Date: Mon, 21 Jan 2019 12:03:01 -0800 Subject: [PATCH 2/2] test: add test & snapshot --- .../test/__snapshots__/rewriter.test.js.snap | 269 ++++++++++++++++++ .../rollup-rewriter/test/rewriter.test.js | 42 ++- .../test/specimens/no-asset-imports/a.css | 1 + .../test/specimens/no-asset-imports/a.js | 5 + .../test/specimens/no-asset-imports/b.css | 1 + .../test/specimens/no-asset-imports/b.js | 5 + .../test/specimens/no-asset-imports/c.css | 1 + .../test/specimens/no-asset-imports/c.js | 5 + .../test/specimens/no-asset-imports/d.js | 1 + 9 files changed, 324 insertions(+), 6 deletions(-) create mode 100644 packages/rollup-rewriter/test/specimens/no-asset-imports/a.css create mode 100644 packages/rollup-rewriter/test/specimens/no-asset-imports/a.js create mode 100644 packages/rollup-rewriter/test/specimens/no-asset-imports/b.css create mode 100644 packages/rollup-rewriter/test/specimens/no-asset-imports/b.js create mode 100644 packages/rollup-rewriter/test/specimens/no-asset-imports/c.css create mode 100644 packages/rollup-rewriter/test/specimens/no-asset-imports/c.js create mode 100644 packages/rollup-rewriter/test/specimens/no-asset-imports/d.js diff --git a/packages/rollup-rewriter/test/__snapshots__/rewriter.test.js.snap b/packages/rollup-rewriter/test/__snapshots__/rewriter.test.js.snap index 40bc34e72..4e2d211e3 100644 --- a/packages/rollup-rewriter/test/__snapshots__/rewriter.test.js.snap +++ b/packages/rollup-rewriter/test/__snapshots__/rewriter.test.js.snap @@ -12,6 +12,275 @@ Array [ ] `; +exports[`rollup-rewriter should only rewrite when necessary: amd 1`] = ` +Object { + "a.js": " +define(['require'], function (require) { 'use strict'; + + var css = { + \\"a\\": \\"a\\" + }; + + console.log(css); + + new Promise(function (resolve, reject) { Promise.all([ + lazyload(\\"./assets/chunk.css\\"), + new Promise(function (resolve, reject) { require(['./chunk.js'], resolve, reject) }) +]) +.then((results) => resolve(results[results.length - 1])) +.catch(reject) }).then(console.log); + +}); +", + "assets/a.css": " +/* packages/rollup-rewriter/test/specimens/no-asset-imports/a.css */ +.a { color: aqua; } +", + "assets/b.css": " +/* packages/rollup-rewriter/test/specimens/no-asset-imports/b.css */ +.b { color: blue; } +", + "assets/chunk.css": " +/* packages/rollup-rewriter/test/specimens/no-asset-imports/c.css */ +.c { color: crimson; } +", + "b.js": " +define(['require'], function (require) { 'use strict'; + + var css = { + \\"b\\": \\"b\\" + }; + + console.log(css); + + new Promise(function (resolve, reject) { require(['./chunk2.js'], resolve, reject) }).then(console.log); + +}); +", + "chunk.js": " +define(['exports'], function (exports) { 'use strict'; + + var css = { + \\"c\\": \\"c\\" + }; + + console.log(css); + + var c$1 = \\"c\\"; + + exports.default = c$1; + +}); +", + "chunk2.js": " +define(['exports'], function (exports) { 'use strict'; + + var d = \\"d\\"; + + exports.default = d; + +}); +", +} +`; + +exports[`rollup-rewriter should only rewrite when necessary: es 1`] = ` +Object { + "a.js": " +var css = { + \\"a\\": \\"a\\" +}; + +console.log(css); + +Promise.all([ + lazyload(\\"./assets/chunk.css\\"), + import('./chunk.js') +]) +.then((results) => results[results.length - 1]).then(console.log); +", + "assets/a.css": " +/* packages/rollup-rewriter/test/specimens/no-asset-imports/a.css */ +.a { color: aqua; } +", + "assets/b.css": " +/* packages/rollup-rewriter/test/specimens/no-asset-imports/b.css */ +.b { color: blue; } +", + "assets/chunk.css": " +/* packages/rollup-rewriter/test/specimens/no-asset-imports/c.css */ +.c { color: crimson; } +", + "b.js": " +var css = { + \\"b\\": \\"b\\" +}; + +console.log(css); + +import('./chunk2.js').then(console.log); +", + "chunk.js": " +var css = { + \\"c\\": \\"c\\" +}; + +console.log(css); + +var c$1 = \\"c\\"; + +export default c$1; +", + "chunk2.js": " +var d = \\"d\\"; + +export default d; +", +} +`; + +exports[`rollup-rewriter should only rewrite when necessary: esm 1`] = ` +Object { + "a.js": " +var css = { + \\"a\\": \\"a\\" +}; + +console.log(css); + +Promise.all([ + lazyload(\\"./assets/chunk.css\\"), + import('./chunk.js') +]) +.then((results) => results[results.length - 1]).then(console.log); +", + "assets/a.css": " +/* packages/rollup-rewriter/test/specimens/no-asset-imports/a.css */ +.a { color: aqua; } +", + "assets/b.css": " +/* packages/rollup-rewriter/test/specimens/no-asset-imports/b.css */ +.b { color: blue; } +", + "assets/chunk.css": " +/* packages/rollup-rewriter/test/specimens/no-asset-imports/c.css */ +.c { color: crimson; } +", + "b.js": " +var css = { + \\"b\\": \\"b\\" +}; + +console.log(css); + +import('./chunk2.js').then(console.log); +", + "chunk.js": " +var css = { + \\"c\\": \\"c\\" +}; + +console.log(css); + +var c$1 = \\"c\\"; + +export default c$1; +", + "chunk2.js": " +var d = \\"d\\"; + +export default d; +", +} +`; + +exports[`rollup-rewriter should only rewrite when necessary: system 1`] = ` +Object { + "a.js": " +System.register([], function (exports, module) { + 'use strict'; + return { + execute: function () { + + var css = { + \\"a\\": \\"a\\" + }; + + console.log(css); + + Promise.all([ + lazyload(\\"./assets/chunk.css\\"), + module.import('./chunk.js') +]) +.then((results) => results[results.length - 1]).then(console.log); + + } + }; +}); +", + "assets/a.css": " +/* packages/rollup-rewriter/test/specimens/no-asset-imports/a.css */ +.a { color: aqua; } +", + "assets/b.css": " +/* packages/rollup-rewriter/test/specimens/no-asset-imports/b.css */ +.b { color: blue; } +", + "assets/chunk.css": " +/* packages/rollup-rewriter/test/specimens/no-asset-imports/c.css */ +.c { color: crimson; } +", + "b.js": " +System.register([], function (exports, module) { + 'use strict'; + return { + execute: function () { + + var css = { + \\"b\\": \\"b\\" + }; + + console.log(css); + + module.import('./chunk2.js').then(console.log); + + } + }; +}); +", + "chunk.js": " +System.register([], function (exports, module) { + 'use strict'; + return { + execute: function () { + + var css = { + \\"c\\": \\"c\\" + }; + + console.log(css); + + var c$1 = exports('default', \\"c\\"); + + } + }; +}); +", + "chunk2.js": " +System.register([], function (exports, module) { + 'use strict'; + return { + execute: function () { + + var d = exports('default', \\"d\\"); + + } + }; +}); +", +} +`; + exports[`rollup-rewriter should support loader & loadfn: amd 1`] = ` Object { "a.js": " diff --git a/packages/rollup-rewriter/test/rewriter.test.js b/packages/rollup-rewriter/test/rewriter.test.js index 77d9a7c9c..d55af4132 100644 --- a/packages/rollup-rewriter/test/rewriter.test.js +++ b/packages/rollup-rewriter/test/rewriter.test.js @@ -76,11 +76,11 @@ describe("rollup-rewriter", () => { const result = await bundle.generate({ format, sourcemap, - + assetFileNames, chunkFileNames, }); - + expect(result).toMatchRollupSnapshot(format); } }); @@ -107,18 +107,48 @@ describe("rollup-rewriter", () => { const result = await bundle.generate({ format, sourcemap, - + assetFileNames, chunkFileNames, }); - + + expect(result).toMatchRollupSnapshot(format); + } + }); + + it("should only rewrite when necessary", async () => { + const bundle = await rollup({ + input : [ + require.resolve("./specimens/no-asset-imports/a.js"), + require.resolve("./specimens/no-asset-imports/b.js"), + ], + plugins : [ + css({ + namer, + map, + }), + rewriter({ + loadfn : "lazyload", + }), + ], + }); + + for(const format of formats) { + const result = await bundle.generate({ + format, + sourcemap, + + assetFileNames, + chunkFileNames, + }); + expect(result).toMatchRollupSnapshot(format); } }); it("should log details in verbose mode", async () => { const { logSnapshot } = logs(); - + const bundle = await rollup({ input : [ require.resolve("./specimens/dynamic-imports/a.js"), @@ -143,7 +173,7 @@ describe("rollup-rewriter", () => { assetFileNames, chunkFileNames, }); - + logSnapshot(); }); }); diff --git a/packages/rollup-rewriter/test/specimens/no-asset-imports/a.css b/packages/rollup-rewriter/test/specimens/no-asset-imports/a.css new file mode 100644 index 000000000..eb93c954b --- /dev/null +++ b/packages/rollup-rewriter/test/specimens/no-asset-imports/a.css @@ -0,0 +1 @@ +.a { color: aqua; } diff --git a/packages/rollup-rewriter/test/specimens/no-asset-imports/a.js b/packages/rollup-rewriter/test/specimens/no-asset-imports/a.js new file mode 100644 index 000000000..041da102c --- /dev/null +++ b/packages/rollup-rewriter/test/specimens/no-asset-imports/a.js @@ -0,0 +1,5 @@ +import css from "./a.css"; + +console.log(css); + +import("./c.js").then(console.log); diff --git a/packages/rollup-rewriter/test/specimens/no-asset-imports/b.css b/packages/rollup-rewriter/test/specimens/no-asset-imports/b.css new file mode 100644 index 000000000..d1a4d005a --- /dev/null +++ b/packages/rollup-rewriter/test/specimens/no-asset-imports/b.css @@ -0,0 +1 @@ +.b { color: blue; } diff --git a/packages/rollup-rewriter/test/specimens/no-asset-imports/b.js b/packages/rollup-rewriter/test/specimens/no-asset-imports/b.js new file mode 100644 index 000000000..9cf9fa11d --- /dev/null +++ b/packages/rollup-rewriter/test/specimens/no-asset-imports/b.js @@ -0,0 +1,5 @@ +import css from "./b.css"; + +console.log(css); + +import("./d.js").then(console.log); diff --git a/packages/rollup-rewriter/test/specimens/no-asset-imports/c.css b/packages/rollup-rewriter/test/specimens/no-asset-imports/c.css new file mode 100644 index 000000000..473e674f1 --- /dev/null +++ b/packages/rollup-rewriter/test/specimens/no-asset-imports/c.css @@ -0,0 +1 @@ +.c { color: crimson; } diff --git a/packages/rollup-rewriter/test/specimens/no-asset-imports/c.js b/packages/rollup-rewriter/test/specimens/no-asset-imports/c.js new file mode 100644 index 000000000..e0fa1cc07 --- /dev/null +++ b/packages/rollup-rewriter/test/specimens/no-asset-imports/c.js @@ -0,0 +1,5 @@ +import css from "./c.css"; + +console.log(css); + +export default "c"; diff --git a/packages/rollup-rewriter/test/specimens/no-asset-imports/d.js b/packages/rollup-rewriter/test/specimens/no-asset-imports/d.js new file mode 100644 index 000000000..987d6d7e4 --- /dev/null +++ b/packages/rollup-rewriter/test/specimens/no-asset-imports/d.js @@ -0,0 +1 @@ +export default "d";