-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[Dialog] Fix TypeScript type for children
#20450
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test for this? It's not obvious to me that this is necessary. The reason being is that non-optional props form their own group in IntelliSense that is suggested before optional props. This helps DX.
Details of bundle changes.Comparing: e751e90...9cd7c3b Details of page changes
|
Perfect, thanks |
Should we systematize the solution?
|
@oliviertassinari happy to PR it. Would you like me to add tests for each one of them as well? Btw, should |
The issue is that I don't understand why this issue seems so important. In most cases the component makes little sense without children so prioritizing it makes sense. Just because something works at runtime doesn't mean the types need to reflect that. |
Prioritising what, sorry?
I think that we should follow what |
IntelliSense and error messages |
So you would like it to error if |
Fixes #20441 (comment)
@eps1lon I think your intention is for it to be optional. Furthermore, if you check e.g. https://github.com/DefinitelyTyped/DefinitelyTyped/blob/7a2c5458a6119695868c4cb801afc237f5a5dc4a/types/react/index.d.ts#L338 you'll see that
children
is specified aschildren?
despite the fact thatReactNode
already includesundefined
.