Typings change in 3.4.34 breaks our dev flow for almost all our plugins

[romainmenke]

see : #1922

---

I agree with the change and I think it is needed to change the types so that nodes can be undefined for at rule statements (e.g. @import, @layer, ...)

But I wonder if a more refined change is possible.
This change is very breaking for us, but not for our users.
Only our own dev flow is affected.

---

Consider this code :

const parent = node.parent;
if (!parent) {
	return false;
}

const siblings = parent.nodes;
const firstSibling = siblings[0]; // errors after `3.4.34`

Here I would assume that nodes is known to be not undefined given that I did node -> parent -> parent.nodes.

[ai]

This change is very breaking for us

Yes, I saw it. But my logic is that previous types was a bug and type is not a core API, but a helper on top of JS code.

Some changes should be made, but we can try to make types smarter to avoid all changes.

Here I would assume that nodes is known to be not undefined given that I did node -> parent -> parent.nodes.

Maybe we can add some type template like Node<Child, IsUndefined> and return Node<Child, false> for Node#parent?

Where else the current types the nodes check will be unnecessary?

[romainmenke]

There are a lot of different cases but I'll try and go through the errors and collect some things that might be improved.

---

Method calls like append take an array of nodes, so it's common to do :

node.append(otherNode.nodes);

If .append (and others) accepts undefined it would just work in all cases.
When undefined is given the method wouldn't do anything.

[ai]

@romainmenke I made PR for node.append(otherNode.nodes) and node.parent.nodes #1924

Hope those hacks will not create new problems 😅

I can release it after your review (also suggest any other changes).

[romainmenke]

I also checked Stylelint and only one thing surfaced there.

There is a helper there :

/**
 * Check if a statement has an block (empty or otherwise).
 *
 * @param {import('postcss').Container} statement
 * @return {boolean} True if `statement` has a block (empty or otherwise)
 */
export default function hasBlock(statement) {
	return statement.nodes !== undefined;
}

But it wasn't immediately clear to me how this could be changed into a real type guard.
ContainerWithChildren isn't exported so I couldn't use this in a type guard.

I don't think that ContainerWithChildren needs to be exported. The smaller we can keep the exported surface, the better.

if (!hasBlock(atRule)) return;

// I expect TypeScript to know that `nodes` isn't undefined
// when `hasBlock` is a working type guard.
console.log(atRule.nodes.length);
//                 ^^^^^ errors

Maybe this is already possible without extra changes?

[romainmenke]

I've tested #1924 and it works really well!

see : csstools/postcss-plugins#1271

Thank you for the super fast changes 🙇

---

I didn't see any changes to test results so I think the runtime changes are good.

The added typings really helped :)
I only had to make a few changes and all were real potential issues, no false positives.

[ai]

Fixed in 8.4.35.

[romainmenke]

Thank you @ai 🎉