Skip to content

Commit

Permalink
fix: don't mutate arrays while iterating
Browse files Browse the repository at this point in the history
This cropped up in a much more complicated codebase at work. Haven't nailed down an actual repro specimen yet but verified this change works there.

Issue was that the outgoing array was being iterated with a .forEach but inside of the loop .removeDependency was being called and that mutates the array. This mean that not all entries in the array would be iterated, leading to incorrect chunking results.

Fix is simple, since dependency-graph already does the right thing and cleans up dependencies when removing a node I can get rid of all my manual cleanup code and just call .removeNode when done iterating.
  • Loading branch information
tivac committed Mar 29, 2019
1 parent d0fdb56 commit 0f63ea2
Show file tree
Hide file tree
Showing 7 changed files with 53 additions and 8 deletions.
19 changes: 11 additions & 8 deletions packages/rollup/chunker.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,25 @@ const merge = (graph, original, target) => {
const incoming = graph.incomingEdges[original];
const outgoing = graph.outgoingEdges[original];

// Add original file reference to target array
graph.getNodeData(target).unshift(original);

// Copy all incoming dependencies from original to target
incoming.forEach((src) => {
graph.removeDependency(src, original);

if(src !== target) {
graph.addDependency(src, target);
if(src === target) {
return;
}

graph.addDependency(src, target);
});

// Copy all outgoing dependencies from original to target
outgoing.forEach((dest) => {
graph.removeDependency(original, dest);

if(dest !== target) {
graph.addDependency(target, dest);
if(dest === target) {
return;
}

graph.addDependency(target, dest);
});

// Bye bye
Expand Down
18 changes: 18 additions & 0 deletions packages/rollup/test/__snapshots__/splitting.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -327,15 +327,33 @@ Object {
.a {
color: aqua;
}
.a2 {
color: azure;
}
/* packages/rollup/test/specimens/common-splitting/shared3.css */
.shared3 {
color: saddlebrown;
}
/* packages/rollup/test/specimens/common-splitting/b.css */
.b {
color: blue;
}
.b2 {
color: blanchedalmond;
}
",
"c.css": "/* packages/rollup/test/specimens/common-splitting/c.css */
.c {
color: cyan;
}
.c2 {
color: crimson;
}",
"shared2.css": "/* packages/rollup/test/specimens/common-splitting/shared2.css */
.shared2 {
color: sandybrown;
}
",
}
`;
Expand Down
6 changes: 6 additions & 0 deletions packages/rollup/test/specimens/common-splitting/a.css
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,9 @@

color: aqua;
}

.a2 {
composes: shared2 from "./shared2.css";

color: azure;
}
6 changes: 6 additions & 0 deletions packages/rollup/test/specimens/common-splitting/b.css
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,9 @@

color: blue;
}

.b2 {
composes: shared3 from "./shared3.css";

color: blanchedalmond;
}
6 changes: 6 additions & 0 deletions packages/rollup/test/specimens/common-splitting/c.css
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
.c {
color: cyan;
}

.c2 {
composes: shared2 from "./shared2.css";

color: crimson;
}
3 changes: 3 additions & 0 deletions packages/rollup/test/specimens/common-splitting/shared2.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.shared2 {
color: sandybrown;
}
3 changes: 3 additions & 0 deletions packages/rollup/test/specimens/common-splitting/shared3.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.shared3 {
color: saddlebrown;
}

0 comments on commit 0f63ea2

Please sign in to comment.