Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Sep 14, 2024
Merged

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab2ff9b
fix: props with children didn't allow for multiple children
hesxenon Sep 13, 2024
bac87fa
Merge branch 'main' into patch-1
rschristian Sep 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compat/src/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -748,7 +748,7 @@ declare namespace React {
): void;

export type PropsWithChildren<P = unknown> = P & {
children?: preact.ComponentChild | undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@hesxenon hesxenon Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@hesxenon hesxenon Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@hesxenon hesxenon Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

children?: preact.ComponentChildren | undefined;
};

export const Children: {
Expand Down
Loading