Skip to content

Commit

Permalink
feat!: make insertions during iteration safe (#295)
Browse files Browse the repository at this point in the history
This change addresses some unexpected behavior when inserting nodes into
a container. For example, assume you have a container containing two
class nodes `.foo.baz`. While iterating that container with `each`,
assume you are processing `.foo` at index 0. Today, the behavior is as
follows:

- `insertBefore(.baz, .bar)` => the next callback is to `.baz` at idx 3
- `insertAfter(.foo, .bar)` => the next callback is to `.baz` at idx 3
- `prepend(.bar)` => the next callback is to `.foo` again at idx 1

With this change, the behavior is the following, respectively:

- the next callback is to `.bar` at idx 2
- the next callback is to `.bar` at idx 2
- the next callback is to `.baz` at idx 3

The newly added tests demonstrate this behavior. I've also removed the
old "container#each (safe iteration)" test, as it now creates an
infinite loop. I'd argue that this is the expected behavior now, though.
  • Loading branch information
mattrberry authored Oct 23, 2024
1 parent 1b1e9c3 commit 4fa6e86
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 14 deletions.
56 changes: 44 additions & 12 deletions src/__tests__/container.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -35,20 +35,52 @@ test('container#each', (t) => {
t.deepEqual(indexes, [0, 1]);
});

test('container#each (safe iteration)', (t) => {
let out = parse('.x, .y', (selectors) => {
selectors.each((selector) => {
selector.parent.insertBefore(
selector,
parser.className({value : 'b'})
);
selector.parent.insertAfter(
selector,
parser.className({value : 'a'})
);
test('container#each (safe iteration w/ insertBefore)', (t) => {
let indexes = [];
let out = parse('.x,.z', (selectors) => {
selectors.each((selector, index) => {
if (index === 0) {
selectors.insertBefore(
selectors.at(1),
parser.className({ value: 'y' })
);
}
indexes.push(index);
});
});
t.deepEqual(out, '.x,.y,.z');
t.deepEqual(indexes, [0, 1, 2]);
});

test('container#each (safe iteration w/ prepend)', (t) => {
let indexes = [];
let out = parse('.y,.z', (selectors) => {
selectors.each((selector, index) => {
if (index === 0) {
selectors.prepend(parser.className({ value: 'x' }));
}
indexes.push(index);
});
});
t.deepEqual(out, '.x,.y,.z');
t.deepEqual(indexes, [0, 2]);
});

test('container#each (safe iteration w/ insertAfter)', (t) => {
let indexes = [];
let out = parse('.x,.z', (selectors) => {
selectors.each((selector, index) => {
if (index === 0) {
selectors.insertAfter(
selector,
parser.className({ value: 'y' })
);
}
indexes.push(index);
});
});
t.deepEqual(out, '.b,.x,.a,.b, .y,.a');
t.deepEqual(out, '.x,.y,.z');
t.deepEqual(indexes, [0, 1, 2]);
});

test('container#each (early exit)', (t) => {
Expand Down
7 changes: 5 additions & 2 deletions src/selectors/container.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ export default class Container extends Node {
prepend (selector) {
selector.parent = this;
this.nodes.unshift(selector);
for ( let id in this.indexes ) {
this.indexes[id]++;
}
return this;
}

Expand Down Expand Up @@ -82,7 +85,7 @@ export default class Container extends Node {
let index;
for ( let id in this.indexes ) {
index = this.indexes[id];
if ( oldIndex <= index ) {
if ( oldIndex < index ) {
this.indexes[id] = index + 1;
}
}
Expand All @@ -100,7 +103,7 @@ export default class Container extends Node {
let index;
for ( let id in this.indexes ) {
index = this.indexes[id];
if ( index <= oldIndex ) {
if ( index >= oldIndex ) {
this.indexes[id] = index + 1;
}
}
Expand Down

0 comments on commit 4fa6e86

Please sign in to comment.