Skip to content
Permalink
Browse files

fix: Improve rollup dependency removal (#442)

* test: try to replicate #441

No luck though, need to get a better repro.

* Got a failing test!

* chore: 🔒 file wut

* 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.

* fix: handle both removal cases

See comment on line 53 for the reasoning, it's goofy.
  • Loading branch information...
tivac committed Jul 5, 2018
1 parent 6ce23fa commit fb6c61ae13cf3dc53ad2ada802506776da7db44a

Some generated files are not rendered by default. Learn more.

Oops, something went wrong.
@@ -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",
@@ -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,13 +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.dependencies(id)
.concat(id)
.forEach((file) => processor.remove(file));
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) => {
@@ -80,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();
@@ -247,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 {
@@ -299,7 +299,7 @@ describe("/rollup.js", () => {
expect(read("dependencies/dependencies.js")).toMatchSnapshot();
expect(read("dependencies/assets/dependencies.css")).toMatchSnapshot();
});

it("should accept an existing processor instance", async () => {
const processor = new Processor({
namer,
@@ -333,6 +333,36 @@ describe("/rollup.js", () => {
expect(read("existing-processor/assets/common.css")).toMatchSnapshot();
});

it("shouldn't over-remove files from an existing processor instance", async () => {
const processor = new Processor({
namer,
map,
});

await processor.file(require.resolve("./specimens/repeated-references/b.css"));

const bundle = await rollup({
input : require.resolve("./specimens/repeated-references/a.js"),
plugins : [
plugin({
processor,
}),
],
});

await bundle.write({
format,
sourcemap,
assetFileNames,

file : `${output}/repeated-references/repeated-references.js`,
});

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", () => {
function checkError(err) {
expect(err.toString()).toMatch("error-plugin:");
@@ -0,0 +1,5 @@
.a {
composes: c from "./c.css";

color: red;
}
@@ -0,0 +1,4 @@
import css from "./a.css";
import b from "./b.js";

console.log(css, b);
@@ -0,0 +1,5 @@
.b {
composes: c from "./c.css";

color: blue;
}
@@ -0,0 +1,3 @@
import css from "./b.css";

export default css;
@@ -0,0 +1,7 @@
.c {
color: cadetblue;
}

.other {
composes: d from "./d.css";
}
@@ -0,0 +1,3 @@
.d {
color: darkblue;
}

0 comments on commit fb6c61a

Please sign in to comment.
You can’t perform that action at this time.