Node.before() inserts after if the new node is already a sibling

[nex3]

Test case:

const postcss = require("postcss");

const root = postcss.parse("@foo; @bar; @baz");
const foo = root.nodes[0];
const baz = root.nodes[2];
baz.before(foo);
root.toString(); // expected: "@bar; @foo; @baz"
                 // actually: "@bar; @baz; @foo"

It looks like the index calculation gets stale once the new node is removed from its original location.

[ai]

Sorry, I need to ask a help with research and PR.

I am spending all my free resources helping people affected by war.

[romainmenke]

👋 willing to take a look in the next few days.

[romainmenke]

Have done some digging and as far as I can tell the Container methods aren't made safe for this scenario.

They are intended and written for adding new nodes.

let root = parse('@alpha; @beta; @gamma')
let alpha = root.nodes[0];
root.push(alpha);

// Just adds one more "@alpha"
// @alpha; @beta; @gamma; @alpha

This is also reflected in the documentation : https://postcss.org/api/#container-before

Insert new node before current node to current node’s parent.

---

That said I think all the methods can be updated with something like this :

function someContainerMethodForInsertion(..., add) {
  if (this.index(add) > -1) {
    this.removeChild(add);
  }

  // existing code

Each method first checks if the node exists and removes it, if needed.

I think this might be a breaking change for some plugins.

---

Update : this comment was incorrect

[ai]

Each method first checks if the node exists and removes it, if needed.
I think this might be a breaking change for some plugins.

Yes, it could be dangerous.

It is better to call remove() in plugin.

[romainmenke]

Taking a more in depth look because there does seem to be some weirdness.

This works and gives the correct final order :

  insertBefore(exist, add) {
    this.removeChild(add); // call before "this.index()"
    exist = this.index(exist)

This is broken and gives an unexpected order :

  insertBefore(exist, add) {
    exist = this.index(exist)
    this.removeChild(add); // call after "this.index()"

So it seems that calling this.index might have more impact than intended.

[nex3]

If the expectation is that Node.before() is only passed a node without an existing parent, imo it should enforce that. If it doesn't enforce it, imo passing in any node with a parent should work according to the function documentation.

[romainmenke]

What I missed before is that the normalize function always detaches a node that will be inserted in a container from its current parent. This comment was incorrect : #1778 (comment)

So @nex3 was absolutely right about :

It looks like the index calculation gets stale once the new node is removed from its original location.

Only insertAfter and insertBefore had local variables with these index values that could go stale. The behaviour also depended on the node order.

for [a, b, c]:

• a.after(c) -> index of a remains the same after removing c
• c.after(a) -> index of c changes after removing a

Changing it would make the behaviour more predictable.

[ai]

@romainmenke fix was released in 8.4.17.

Can we close an issue?

[romainmenke]

@ai Did a few checks throughout the ecosystem and found that 8.4.17 affects whitespace.

I don't think this is an issue as the whitespace change doesn't affect the actual CSS to the best of my knowledge.

I've opened pull requests to update test results :

• update tests results for postcss 8.4.17 autoprefixer#1481
• update tests results for postcss 8.4.17 postcss-import#503

But these also give you an idea of the effect in case you do think this is something that requires further action.

---

If no action is required we can close this :)
Thank you again for splitting your focus for this.

[romainmenke]

@nex3 Have you had a chance to try out the latest version to verify that this is fixed?

[romainmenke]

I think we can close this.