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.

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

Update ComponentChild types #4432

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Update ComponentChild types #4432

wants to merge 3 commits into from

Conversation

sant123
Copy link

@sant123 sant123 commented Jul 6, 2024

The object type is pretty wide and does not follow ReactNode types, so this change is to reflect the type from React.

Follows #1116

The type `object` is pretty wide and does not follow ReactNode types, so this change is to reflect the type from React.
@rschristian
Copy link
Member

Looks like this breaks a few TS tests.

does not follow ReactNode types, so this change is to reflect the type from React.

For what it's worth, we don't aim to match React's types in core necessarily, only in compat, and even that is more about avoiding conflict than matching in strictness. We're different frameworks with different implementations, we do accept different types in some circumstances.

@sant123
Copy link
Author

sant123 commented Jul 6, 2024

Looks like this breaks a few TS tests.

does not follow ReactNode types, so this change is to reflect the type from React.

For what it's worth, we don't aim to match React's types in core necessarily, only in compat, and even that is more about avoiding conflict than matching in strictness. We're different frameworks with different implementations, we do accept different types in some circumstances.

Fair enough, for the failed tests the issue is when passing functions as children in components (that is found in Context.Consumer). I have uploaded a fix for that so the tests should be passing.

@coveralls
Copy link

Coverage Status

coverage: 99.61%. remained the same
when pulling 2d439ac on sant123:patch-1
into 82ab555 on preactjs:main.

src/index.d.ts Outdated
| string
| number
| Iterable<ComponentChild>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we support any iterables as component children. Can't find code in our source that explicitly invokes the iterator protocol.

Copy link
Author

Choose a reason for hiding this comment

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

So do you think is better the Array type here?

Comment on lines 44 to 54
export type ComponentChild =
| VNode<any>
| object
| string
| number
| Array<ComponentChild>
| bigint
| boolean
| null
| undefined;
| undefined
| Function;
export type ComponentChildren = ComponentChild[] | ComponentChild;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder, do we really need to alter the type here? I do generally prefer ComponentChild being singular, with ComponentChildren being used if an array of children is needed.

If it's a compat issue, we can simply alter the ReactNode type instead?

Choose a reason for hiding this comment

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

looks kinda weird - with this change, ComponentChild and ComponentChildren are the same type...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants