From 11b18e85d40775f8258481aa8a3d11b20782d174 Mon Sep 17 00:00:00 2001 From: Pat Cavit Date: Sun, 1 Jul 2018 23:46:36 -0700 Subject: [PATCH 1/5] test: try to replicate #441 No luck though, need to get a better repro. --- package-lock.json | 12 +++++----- package.json | 2 +- .../test/__snapshots__/rollup.test.js.snap | 17 ++++++++++++++ packages/rollup/test/rollup.test.js | 23 +++++++++++++++++++ .../test/specimens/file-treeshaking/a.css | 1 + .../test/specimens/file-treeshaking/a.js | 4 ++++ .../test/specimens/file-treeshaking/b.css | 1 + .../test/specimens/file-treeshaking/b.js | 7 ++++++ 8 files changed, 60 insertions(+), 7 deletions(-) create mode 100644 packages/rollup/test/specimens/file-treeshaking/a.css create mode 100644 packages/rollup/test/specimens/file-treeshaking/a.js create mode 100644 packages/rollup/test/specimens/file-treeshaking/b.css create mode 100644 packages/rollup/test/specimens/file-treeshaking/b.js diff --git a/package-lock.json b/package-lock.json index 1be352238..c4a9194de 100644 --- a/package-lock.json +++ b/package-lock.json @@ -831,9 +831,9 @@ "dev": true }, "@types/node": { - "version": "10.3.5", - "resolved": "https://registry.npmjs.org/@types/node/-/node-10.3.5.tgz", - "integrity": "sha512-6lRwZN0Y3TuglwaaZN2XPocobmzLlhxcqDjKFjNYSsXG/TFAGYkCqkzZh4+ms8iTHHQE6gJXLHPV7TziVGeWhg==", + "version": "10.5.1", + "resolved": "https://registry.npmjs.org/@types/node/-/node-10.5.1.tgz", + "integrity": "sha512-AFLl1IALIuyt6oK4AYZsgWVJ/5rnyzQWud7IebaZWWV3YmgtPZkQmYio9R5Ze/2pdd7XfqF5bP+hWS11mAKoOQ==", "dev": true }, "@webassemblyjs/ast": { @@ -10480,9 +10480,9 @@ } }, "rollup": { - "version": "0.61.2", - "resolved": "https://registry.npmjs.org/rollup/-/rollup-0.61.2.tgz", - "integrity": "sha512-NN7GaX3c9I3oz+CjEex7+JbnSxrLMTBXNRThI4qdN2Tq0K/7RiedlEzOHyuVAHPZKvQeoPiRioTWL3xhmN+oCg==", + "version": "0.62.0", + "resolved": "https://registry.npmjs.org/rollup/-/rollup-0.62.0.tgz", + "integrity": "sha512-mZS0aIGfYzuJySJD78znu9/hCJsNfBzg4lDuZGMj0hFVcYHt2evNRHv8aqiu9/w6z6Qn8AQoVl4iyEjDmisGeA==", "dev": true, "requires": { "@types/estree": "0.0.39", diff --git a/package.json b/package.json index 84c63b244..3527fd68b 100644 --- a/package.json +++ b/package.json @@ -39,7 +39,7 @@ "lint-staged": "^7.0.4", "modular-css-core": "file:./packages/core", "pegjs": "^0.10.0", - "rollup": "^0.61.2", + "rollup": "^0.62.0", "rollup-plugin-svelte": "^4.1.0", "shelljs": "^0.8.1", "svelte": "^2.8.1", diff --git a/packages/rollup/test/__snapshots__/rollup.test.js.snap b/packages/rollup/test/__snapshots__/rollup.test.js.snap index d0ffbf5e4..7c4b340ca 100644 --- a/packages/rollup/test/__snapshots__/rollup.test.js.snap +++ b/packages/rollup/test/__snapshots__/rollup.test.js.snap @@ -120,6 +120,23 @@ exports[`/rollup.js should correctly pass to/from params for relative paths 1`] " `; +exports[`/rollup.js should exclude CSS files that were removed by treeshaking 1`] = ` +"var css = { + \\"a\\": \\"a\\" +}; + +console.log(css); +" +`; + +exports[`/rollup.js should exclude CSS files that were removed by treeshaking 2`] = ` +"/* packages/rollup/test/specimens/file-treeshaking/a.css */ +.a { color: red; } +/* packages/rollup/test/specimens/file-treeshaking/b.css */ +.b { color: blue; } +" +`; + exports[`/rollup.js should generate CSS 1`] = ` "/* packages/rollup/test/specimens/simple.css */ .fooga { diff --git a/packages/rollup/test/rollup.test.js b/packages/rollup/test/rollup.test.js index 5d3540380..151bd20cc 100644 --- a/packages/rollup/test/rollup.test.js +++ b/packages/rollup/test/rollup.test.js @@ -299,6 +299,29 @@ describe("/rollup.js", () => { expect(read("dependencies/dependencies.js")).toMatchSnapshot(); expect(read("dependencies/assets/dependencies.css")).toMatchSnapshot(); }); + + it("should exclude CSS files that were removed by treeshaking", async () => { + const bundle = await rollup({ + input : require.resolve("./specimens/file-treeshaking/a.js"), + plugins : [ + plugin({ + namer, + map, + }), + ], + }); + + await bundle.write({ + format, + assetFileNames, + sourcemap, + + file : `${output}/file-treeshaking/file-treeshaking.js`, + }); + + expect(read("file-treeshaking/file-treeshaking.js")).toMatchSnapshot(); + expect(read("file-treeshaking/assets/file-treeshaking.css")).toMatchSnapshot(); + }); it("should accept an existing processor instance", async () => { const processor = new Processor({ diff --git a/packages/rollup/test/specimens/file-treeshaking/a.css b/packages/rollup/test/specimens/file-treeshaking/a.css new file mode 100644 index 000000000..f805c8820 --- /dev/null +++ b/packages/rollup/test/specimens/file-treeshaking/a.css @@ -0,0 +1 @@ +.a { color: red; } diff --git a/packages/rollup/test/specimens/file-treeshaking/a.js b/packages/rollup/test/specimens/file-treeshaking/a.js new file mode 100644 index 000000000..8458932c2 --- /dev/null +++ b/packages/rollup/test/specimens/file-treeshaking/a.js @@ -0,0 +1,4 @@ +import css from "./a.css"; +import b from "./b.js"; + +console.log(css); diff --git a/packages/rollup/test/specimens/file-treeshaking/b.css b/packages/rollup/test/specimens/file-treeshaking/b.css new file mode 100644 index 000000000..d1a4d005a --- /dev/null +++ b/packages/rollup/test/specimens/file-treeshaking/b.css @@ -0,0 +1 @@ +.b { color: blue; } diff --git a/packages/rollup/test/specimens/file-treeshaking/b.js b/packages/rollup/test/specimens/file-treeshaking/b.js new file mode 100644 index 000000000..3db1e2df6 --- /dev/null +++ b/packages/rollup/test/specimens/file-treeshaking/b.js @@ -0,0 +1,7 @@ +import css from "./b.css"; + +function echo() { + console.log(css); +} + +export default echo; From 892d30871e5279344db33590dfe10de9cfec65ac Mon Sep 17 00:00:00 2001 From: Pat Cavit Date: Thu, 5 Jul 2018 10:31:50 -0700 Subject: [PATCH 2/5] Got a failing test! --- .../test/__snapshots__/rollup.test.js.snap | 56 +++++++++++++------ packages/rollup/test/rollup.test.js | 45 ++++++++------- .../test/specimens/file-treeshaking/a.css | 1 - .../test/specimens/file-treeshaking/b.css | 1 - .../test/specimens/file-treeshaking/b.js | 7 --- .../test/specimens/repeated-references/a.css | 5 ++ .../a.js | 2 +- .../test/specimens/repeated-references/b.css | 5 ++ .../test/specimens/repeated-references/b.js | 3 + .../test/specimens/repeated-references/c.css | 7 +++ .../test/specimens/repeated-references/d.css | 3 + 11 files changed, 89 insertions(+), 46 deletions(-) delete mode 100644 packages/rollup/test/specimens/file-treeshaking/a.css delete mode 100644 packages/rollup/test/specimens/file-treeshaking/b.css delete mode 100644 packages/rollup/test/specimens/file-treeshaking/b.js create mode 100644 packages/rollup/test/specimens/repeated-references/a.css rename packages/rollup/test/specimens/{file-treeshaking => repeated-references}/a.js (71%) create mode 100644 packages/rollup/test/specimens/repeated-references/b.css create mode 100644 packages/rollup/test/specimens/repeated-references/b.js create mode 100644 packages/rollup/test/specimens/repeated-references/c.css create mode 100644 packages/rollup/test/specimens/repeated-references/d.css diff --git a/packages/rollup/test/__snapshots__/rollup.test.js.snap b/packages/rollup/test/__snapshots__/rollup.test.js.snap index 7c4b340ca..a65d9416e 100644 --- a/packages/rollup/test/__snapshots__/rollup.test.js.snap +++ b/packages/rollup/test/__snapshots__/rollup.test.js.snap @@ -120,23 +120,6 @@ exports[`/rollup.js should correctly pass to/from params for relative paths 1`] " `; -exports[`/rollup.js should exclude CSS files that were removed by treeshaking 1`] = ` -"var css = { - \\"a\\": \\"a\\" -}; - -console.log(css); -" -`; - -exports[`/rollup.js should exclude CSS files that were removed by treeshaking 2`] = ` -"/* packages/rollup/test/specimens/file-treeshaking/a.css */ -.a { color: red; } -/* packages/rollup/test/specimens/file-treeshaking/b.css */ -.b { color: blue; } -" -`; - exports[`/rollup.js should generate CSS 1`] = ` "/* packages/rollup/test/specimens/simple.css */ .fooga { @@ -264,6 +247,45 @@ console.log(css); } `; +exports[`/rollup.js shouldn't over-remove files from an existing processor instance 1`] = ` +"var css = { + \\"a\\": \\"c a\\" +}; + +var css$1 = { + \\"b\\": \\"c b\\" +}; + +console.log(css, css$1); +" +`; + +exports[`/rollup.js shouldn't over-remove files from an existing processor instance 2`] = ` +"/* packages/rollup/test/specimens/repeated-references/a.css */ +.a { + + color: red; +} +/* packages/rollup/test/specimens/repeated-references/b.css */ +.b { + + color: blue; +} +" +`; + +exports[`/rollup.js shouldn't over-remove files from an existing processor instance 3`] = ` +"/* packages/rollup/test/specimens/repeated-references/d.css */ +.d { + color: darkblue; +} +/* packages/rollup/test/specimens/repeated-references/c.css */ +.c { + color: cadetblue; +} +" +`; + exports[`/rollup.js watch should correctly add new css files in watch mode when files change 1`] = ` "/* packages/rollup/test/output/one.css */ .mc19ef5610_one { diff --git a/packages/rollup/test/rollup.test.js b/packages/rollup/test/rollup.test.js index 151bd20cc..89dd0ee07 100644 --- a/packages/rollup/test/rollup.test.js +++ b/packages/rollup/test/rollup.test.js @@ -300,43 +300,49 @@ describe("/rollup.js", () => { expect(read("dependencies/assets/dependencies.css")).toMatchSnapshot(); }); - it("should exclude CSS files that were removed by treeshaking", async () => { + it("should accept an existing processor instance", async () => { + const processor = new Processor({ + namer, + map, + }); + + await processor.string("./packages/rollup/test/specimens/fake.css", dedent(` + .fake { + color: yellow; + } + `)); + const bundle = await rollup({ - input : require.resolve("./specimens/file-treeshaking/a.js"), + input : require.resolve("./specimens/simple.js"), plugins : [ plugin({ - namer, - map, + processor, }), ], }); await bundle.write({ format, - assetFileNames, sourcemap, - - file : `${output}/file-treeshaking/file-treeshaking.js`, + assetFileNames, + + file : `${output}/existing-processor/existing-processor.js`, }); - expect(read("file-treeshaking/file-treeshaking.js")).toMatchSnapshot(); - expect(read("file-treeshaking/assets/file-treeshaking.css")).toMatchSnapshot(); + expect(read("existing-processor/assets/existing-processor.css")).toMatchSnapshot(); + expect(read("existing-processor/assets/common.css")).toMatchSnapshot(); }); - it("should accept an existing processor instance", async () => { + it("shouldn't over-remove files from an existing processor instance", async () => { const processor = new Processor({ namer, map, }); - await processor.string("./packages/rollup/test/specimens/fake.css", dedent(` - .fake { - color: yellow; - } - `)); + await processor.file(require.resolve("./specimens/repeated-references/b.css")); const bundle = await rollup({ - input : require.resolve("./specimens/simple.js"), + input : require.resolve("./specimens/repeated-references/a.js"), plugins : [ plugin({ processor, @@ -349,11 +355,12 @@ describe("/rollup.js", () => { sourcemap, assetFileNames, - file : `${output}/existing-processor/existing-processor.js`, + file : `${output}/repeated-references/repeated-references.js`, }); - expect(read("existing-processor/assets/existing-processor.css")).toMatchSnapshot(); - expect(read("existing-processor/assets/common.css")).toMatchSnapshot(); + expect(read("repeated-references/repeated-references.js")).toMatchSnapshot(); + expect(read("repeated-references/assets/repeated-references.css")).toMatchSnapshot(); + expect(read("repeated-references/assets/common.css")).toMatchSnapshot(); }); describe("errors", () => { diff --git a/packages/rollup/test/specimens/file-treeshaking/a.css b/packages/rollup/test/specimens/file-treeshaking/a.css deleted file mode 100644 index f805c8820..000000000 --- a/packages/rollup/test/specimens/file-treeshaking/a.css +++ /dev/null @@ -1 +0,0 @@ -.a { color: red; } diff --git a/packages/rollup/test/specimens/file-treeshaking/b.css b/packages/rollup/test/specimens/file-treeshaking/b.css deleted file mode 100644 index d1a4d005a..000000000 --- a/packages/rollup/test/specimens/file-treeshaking/b.css +++ /dev/null @@ -1 +0,0 @@ -.b { color: blue; } diff --git a/packages/rollup/test/specimens/file-treeshaking/b.js b/packages/rollup/test/specimens/file-treeshaking/b.js deleted file mode 100644 index 3db1e2df6..000000000 --- a/packages/rollup/test/specimens/file-treeshaking/b.js +++ /dev/null @@ -1,7 +0,0 @@ -import css from "./b.css"; - -function echo() { - console.log(css); -} - -export default echo; diff --git a/packages/rollup/test/specimens/repeated-references/a.css b/packages/rollup/test/specimens/repeated-references/a.css new file mode 100644 index 000000000..02f54c7c1 --- /dev/null +++ b/packages/rollup/test/specimens/repeated-references/a.css @@ -0,0 +1,5 @@ +.a { + composes: c from "./c.css"; + + color: red; +} diff --git a/packages/rollup/test/specimens/file-treeshaking/a.js b/packages/rollup/test/specimens/repeated-references/a.js similarity index 71% rename from packages/rollup/test/specimens/file-treeshaking/a.js rename to packages/rollup/test/specimens/repeated-references/a.js index 8458932c2..8479cfd57 100644 --- a/packages/rollup/test/specimens/file-treeshaking/a.js +++ b/packages/rollup/test/specimens/repeated-references/a.js @@ -1,4 +1,4 @@ import css from "./a.css"; import b from "./b.js"; -console.log(css); +console.log(css, b); diff --git a/packages/rollup/test/specimens/repeated-references/b.css b/packages/rollup/test/specimens/repeated-references/b.css new file mode 100644 index 000000000..37ce050fc --- /dev/null +++ b/packages/rollup/test/specimens/repeated-references/b.css @@ -0,0 +1,5 @@ +.b { + composes: c from "./c.css"; + + color: blue; +} diff --git a/packages/rollup/test/specimens/repeated-references/b.js b/packages/rollup/test/specimens/repeated-references/b.js new file mode 100644 index 000000000..cc61d65d9 --- /dev/null +++ b/packages/rollup/test/specimens/repeated-references/b.js @@ -0,0 +1,3 @@ +import css from "./b.css"; + +export default css; diff --git a/packages/rollup/test/specimens/repeated-references/c.css b/packages/rollup/test/specimens/repeated-references/c.css new file mode 100644 index 000000000..e38c48644 --- /dev/null +++ b/packages/rollup/test/specimens/repeated-references/c.css @@ -0,0 +1,7 @@ +.c { + color: cadetblue; +} + +.other { + composes: d from "./d.css"; +} diff --git a/packages/rollup/test/specimens/repeated-references/d.css b/packages/rollup/test/specimens/repeated-references/d.css new file mode 100644 index 000000000..82df04199 --- /dev/null +++ b/packages/rollup/test/specimens/repeated-references/d.css @@ -0,0 +1,3 @@ +.d { + color: darkblue; +} From 1c084a84c3db3cd3984fc994ccfee9103991fbcf Mon Sep 17 00:00:00 2001 From: Pat Cavit Date: Thu, 5 Jul 2018 10:32:08 -0700 Subject: [PATCH 3/5] chore: :lock: file wut --- package-lock.json | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index c4a9194de..0efc9440c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4807,12 +4807,14 @@ "balanced-match": { "version": "1.0.0", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "brace-expansion": { "version": "1.1.11", "bundled": true, "dev": true, + "optional": true, "requires": { "balanced-match": "^1.0.0", "concat-map": "0.0.1" @@ -4832,7 +4834,8 @@ "concat-map": { "version": "0.0.1", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "console-control-strings": { "version": "1.1.0", From ff3e73efb3173170f7580ab64b2ce640ad3f1492 Mon Sep 17 00:00:00 2001 From: Pat Cavit Date: Thu, 5 Jul 2018 10:37:21 -0700 Subject: [PATCH 4/5] fix: Only remove the file and its dependents Not sure why I changed it to the file and all its dependencies instead of just the default "file and its dependants" behavior, but that was the WRONG choice. --- packages/rollup/rollup.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/rollup/rollup.js b/packages/rollup/rollup.js index 8f38b9138..5675ca38d 100644 --- a/packages/rollup/rollup.js +++ b/packages/rollup/rollup.js @@ -46,9 +46,7 @@ module.exports = function(opts) { // If the file is being re-processed we need to remove it to // avoid cache staleness issues if(id in processor.files) { - processor.dependencies(id) - .concat(id) - .forEach((file) => processor.remove(file)); + processor.remove(id); } return processor.string(id, code).then((result) => { From 6ad3858bf84efcf57b29e266ea534fa9a7157a7f Mon Sep 17 00:00:00 2001 From: Pat Cavit Date: Thu, 5 Jul 2018 11:30:41 -0700 Subject: [PATCH 5/5] fix: handle both removal cases See comment on line 53 for the reasoning, it's goofy. --- packages/rollup/rollup.js | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/packages/rollup/rollup.js b/packages/rollup/rollup.js index 5675ca38d..66e6ec120 100644 --- a/packages/rollup/rollup.js +++ b/packages/rollup/rollup.js @@ -34,6 +34,8 @@ module.exports = function(opts) { const filter = utils.createFilter(options.include, options.exclude); const processor = options.processor || new Processor(options); + + let runs = 0; return { name : "modular-css-rollup", @@ -42,11 +44,20 @@ module.exports = function(opts) { if(!filter(id)) { return null; } - + // If the file is being re-processed we need to remove it to // avoid cache staleness issues if(id in processor.files) { - processor.remove(id); + let files = [ id ]; + + // First time build should only remove the file and any files that depend on it + // but watchs update need to clear out the file, any files that depend on it, + // AND any files it depends on + if(runs) { + files = processor.dependencies(id).concat(files); + } + + files.forEach((file) => processor.remove(file)); } return processor.string(id, code).then((result) => { @@ -78,6 +89,11 @@ module.exports = function(opts) { }); }, + // Track # of runs since remove functionality needs to change + buildEnd() { + runs++; + }, + async generateBundle(outputOptions, bundles) { const usage = new Map(); const common = new Map();