fix: react props with children didn't allow for multiple children

[hesxenon]

in some weird cases this led to wrong errors being displayed. Sadly I don't fully understand why, but defining a component (e.g. a CE) with React.PropsWithChildren and providing multiple children as well as non-existent props led to typescript complaining about Element is not assignable to string.

So e.g.

function Comp(_: React.PropsWithChildren) {
  return <></>;
}

const x = (
  <Comp foo="bar">
    <div />
    <div />
  </Comp>
);

is invalid.

const x = (
  <Comp foo="bar">
    <div />
  </Comp>
)

however yields the expected error that foo is not a prop on Comp.

While I do think that this is not preacts error I don't see why intrinsic elements should be able to receive multiple children while explicitly typed components should not?

[coveralls]

coverage: 99.488%. remained the same
when pulling bac87fa on hesxenon:patch-1
into 8d0ee49 on preactjs:main.

[rschristian]

React's type seems to be ReactNode, rather than ReactNode[] (which they do use elsewhere). As ComponentChildren is ComponentChild | ComponentChild[], I'm not quite sure this change is correct.

Does this work correctly in React? It's very possible we have an issue elsewhere in the types.

[hesxenon]

well, yes and no: ReactNode is an alias of ComponentChild but does not include Iterable<ComponentChild> like reacts ReactNode does does and simply extending that union in preact/compat only isn't possible the way it's written currently. I thought this is the minimal change but I agree, imho the correct change would be to drop ComponentChildren, rename ComponentChild to PreactNode and include Iterable<PreactNode> in it. But that's not exactly a small change...

[rschristian]

We don't support iterables, the type is correct there FWIW.

[hesxenon]

wdym "we don't support iterables"? You have arrays in your types, arrays are iterables? Or did you mean PreactNode[] instead of Iterable<PreactNode>?

[hesxenon]

as for "does this work correctly in react" - well, it's a type change? Ofc react still works. And if you're using react (not preact) then this issue wouldn't arise in the first place?

Kinda confused what you mean to say here... changing this solves the described issue and doesn't break anything else in the rather large project that I'm currently migrating from react to preact, so... yes? It works in a "react like context"?

EDIT: ah, you probably meant "does the issue above arise in react" - no, it doesn't probably because ReactNode includes iterables.

[rschristian]

wdym "we don't support iterables"? You have arrays in your types, arrays are iterables?

Arrays are iterable, but not all iterables are arrays; Map, Set, Uint32Array, and other structures are iterable too.

as for "does this work correctly in react" - well, it's a type change? Ofc react still works.

I'm asking if your type (for your component) works in React -- I've always used ReactNode[]/ComponentChildren for multiple nodes, personally.

[hesxenon]

Yes it does, we're using this heavily throughout multiple large projects and never had any problems until we migrated to preact (which we did because react is basically incompatible with CEs and probably will continue to be)

[rschristian]

After taking a look, I don't think this is the correct solution.

The generic of PropsWithChildren defaults to unknown, which seems to be the real cause for issue. If we switch it to this:

interface CompProps {
    foo: string;
}
function Comp(_: React.PropsWithChildren<CompProps>) {
  return <></>;
}

...then the issue will go away, and multiple children are a-okay.

I believe this is because any union of unknown (as PropsWithChildren, sans generic, produces unknown & { children?: preact.ComponentChild | undefined; } in Preact) collapses to unknown, essentially ignoring the latter half of the union altogether.

That being said, there are still some weird interactions that I don't quite understand. If we take the component definition above, and use it like so:

const x = (
  <Comp foo="bar" baz={0}>
    <div />
    <div />
  </Comp>
);

...the Type 'Element' is not assignable to type 'string' error that you mentioned will reappear on the child divs, though the issue is due to that extraneous baz prop instead. Why the extra prop results in an error on the children, I have no clue at the moment.

---

I'm wondering if we do have a deeper types issue somewhere, as this seems a bit funky & confusing indeed. Maybe any is a better default for that generic? Not sure.

[hesxenon]

any union with unknown collapses to unknown

sorry, that is wrong.

Also I must point out that your reproduction is flawed in that it produces exactly the same issue as I originally described: you're passing multiple children which is fine as long as there aren't excess props. As soon as the excess prop appears the children must be of type string (since that is the only type that allows for multiple lines to appear as a single child maybe?)

The issue at hand is that preacts definition of a "Node" diverges from reacts definition - adding ComponentChild[] to the CompnentChild definition would align it again. The question is just how big of a change is this for preact itself and is it preferrable in this situation to implement a "larger than necessary" that is more correct.

Also, the "we don't support iterables" thing might be good to mention in the "differences to react" section. But I have to admit I've never seen the use of other iterables as children though I can imagine some use cases for that...

Btw, in said "differences to react" section I just read it states

In Preact, props.children is either a Virtual DOM node, an empty value like null, or an Array of Virtual DOM nodes

which should (currently) be appended with: "...unless you switch from react and have components that utilize React.PropsWithChildren".

[rschristian]

any union with unknown collapses to unknown

sorry, that is wrong.

& is an intersection, not a union! My bad, mixed up syntax & names. You're totally right.

Also I must point out that your reproduction is flawed in that it produces exactly the same issue as I originally described: you're passing multiple children which is fine as long as there aren't excess props.

My reproduction assumes you want to adhere to your types, yes. Your example should error, albeit, produce a different message. The reproduction was intended to show corrected use.

adding ComponentChild[] to the CompnentChild definition would align it again.

Fair enough, we'll land this for now & address it later if it causes issues.

Generally, we try to stick to React's types as closes as possible for easy comparison later -- ideally TS would have some better mechanism for patching/overriding types, but as it is, we have to re-implement all those asked for. That's why I'm somewhat wary of altering too much.

As soon as the excess prop appears the children must be of type string (since that is the only type that allows for multiple lines to appear as a single child maybe?)

This is the more interesting issue.

[hesxenon]

after some digging I have a hunch (at least) as to why this is happening. Preacts JSX runtime is typing the jsx function with a single ComponentChild and the jsxs function with ComponentChildren which, as far as I can tell, aligns with the initial changes document from react. It's a bit difficult to find an actual specification of typescripts behaviour here, but as far as I can tell by looking at their compiler code they choose the jsxs function when they determine the children as "static" and the jsx function otherwise. Now, I have a feeling that this check somehow determines the children are "dynamic" as soon as the props can't be reliably determined (which must be the case with excess props?? bivariance maybe?) which leads to jsx being chosen over jsxs and thus the children must be singular...

[hesxenon]

damnit, I just wanted to write "if this were my project I'd hold off merging this until I have some clarification from the typescript team"^^

[rschristian]

Huh, interesting.

damnit, I just wanted to write "if this were my project I'd hold off merging this until I have some clarification from the typescript team"^^

Shouldn't be a problem; this will go out next release, at the very least fixing your components/app. If people do have issues or we find out more later, we can always revert or make changes. Not a worry.