Make insertions during iteration safe

[mattrbeck]

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 2
• insertAfter(.foo, .bar) => the next callback is to .baz at idx 2
• 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 1
• the next callback is to .bar at idx 1
• the next callback is to .baz at idx 2

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.

[alexander-akait]

/cc @romainmenke I'm afraid this might be a breaking change, what do you think?

[romainmenke]

Yes, this is a breaking change.
This would for example break postcss-nesting.

as it now creates an infinite loop. I'd argue that this is the expected behavior now, though.

Can we somehow avoid rolling out an update that results in infinite loops for code that ran fine before?

---

We received a similar request from Google some time ago: csstools/postcss-plugins#1202

We eventually switched to a more nuanced way of iterating nodes:
https://github.com/csstools/postcss-plugins/blob/main/packages/css-parser-algorithms/src/util/walker-index-generator.ts

[mattrbeck]

Thank you for the quick follow-up!

Which change specifically is breaking? I'd imagine the change to insertAfter, since that's what breaks the deleted test? While I feel like the changed behavior matches my expectations more closely, I'm happy to split that off and discuss it separately. The changes to insertBefore and prepend can definitely stand on their own.

If this breaks nesting, maybe nesting could be made more resilient first so this change wouldn't result in an infinite loop? To me that feels better than rather than relying on (what I consider to be) surprising iterating behavior.

Please let me know if my assumptions are wrong, though :) I'm very new to using PostCSS.

(Apologies for any typos; sending this from mobile)

[romainmenke]

I haven't pinpointed exactly why postcss-nesting now behaves differently.

It isn't stuck in an infinite loop, it produces different outcomes, effectively breaking the plugin because it no longer correctly transpiles nested CSS selectors.

If this breaks nesting, maybe nesting could be made more resilient first

Definitely, but that would still require making this a semver major to ensure that anyone on older versions wouldn't accidentally get this behavior change when updating transitive dependencies.

I am open to changes and willing to deal with the downstream impact as far as those plugins are maintained by me :)

---

The infinite loop comment is separate from the issues with any individual plugin.
Such a change (higher chance of infinite loops) is always a bit alarming to me.

Arguably the removed test should have been:

test('container#each (safe iteration)', (t) => {
    let out = parse('.x, .y', (selectors) => {
        selectors.each((selector) => {
            if (selector.type === 'class' && selector.value !== 'x' && selector.value !== 'y') {
                return;
            }

            selector.parent.insertBefore(
                selector,
                parser.className({value: 'b'})
            );
            selector.parent.insertAfter(
                selector,
                parser.className({value: 'a'})
            );
        });
    });
    t.deepEqual(out, '.b,.x,.a,.b, .y,.a');
});

It is highly unusual to make AST mutations and inserting new nodes without checking what the current node is.

I agree that it is fair that "insert one more node after the current node" will always result in an infinite loop if there are no constraints. But somewhere someone will have made such a mistake.

---

Is it correct that this change also aligns postcss-selector-parser with PostCSS itself?
As far as I can tell it would make their behaviors more consistent.

https://github.com/postcss/postcss/blob/main/lib/container.js

[romainmenke]

I haven't pinpointed exactly why postcss-nesting now behaves differently.

We use prepend in a sorting step where we sort all nodes in a given selector after a replacement.

We do this to ensure that we don't produce invalid selectors after serializing. e.g. .foo and bar resulting in .foobar instead of the expected bar.foo.

Because prepend now increments the indices, the iterators will advance even when we don't expect them to.

[mattrbeck]

Wouldn't the old behavior still be buggy in that case where you prepend multiple times in the same iteration, though? For example, if you have a selector .baz and you prepend(.bar); prepend(.foo) in the same iteration, you'll end up on .bar for the second iteration. Maybe you didn't prepend twice in the same iteration before, though?

[romainmenke]

We called prepend many times, but with preexisting nodes, to sort these.

---

I am not advocating for or defending the old behavior :)
I was only giving some context on why it is a breaking change for us.

I've gone ahead and changed our code so that we do simpler array mutations and don't use the AST methods when sorting.

I think this change is fine, if published as a semver major.
Especially if this brings postcss-selector-parser more in line with postcss itself.

[alexander-akait]

@romainmenke is will be fine to make major release for your dependent packages, because we are one of the main consumers

[romainmenke]

I've also just checked cssnano and stylelint.
Both work just fine with these changes.

So I think we can move forwards with this :)