-
Notifications
You must be signed in to change notification settings - Fork 8k
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
Add explicit children types #181257
Add explicit children types #181257
Conversation
/ci |
/ci |
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.
desk tested and code LGTM for the Threat Intelligence Investigations team! Thanks for making this changes!
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.
Thanks for the changes @patrykkopycinski! I have checked the types and can confirm that changes cause no issues with our current version of React. Approving on behalf of "security-detection-rule-management".
Couple questions:
- Why is
children
optional in some cases and required in others? - What's the next step for v18 upgrade? Do we need to upgrade Kibana simultaneously with EUI so that they are compatible?
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.
Core changes LGTM
Thank you for the feedback @jgowdyelastic @cnasikas @nikitaindik , I have aligned all the changes to use |
...cation/data_frame_analytics/pages/analytics_creation/components/details_step/description.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/components/page_header/page_header.tsx
Outdated
Show resolved
Hide resolved
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.
ResponseOps changes LGTM!
…s/analytics_creation/components/details_step/description.tsx Co-authored-by: James Gowdy <jgowdy@elastic.co>
…bana into chore/react-18-props # Conflicts: # x-pack/plugins/ml/public/application/data_frame_analytics/pages/analytics_creation/components/details_step/description.tsx
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Any counts in public APIs
Async chunks
Canvas Sharable Runtime
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: |
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.
There are still quite a lot of places in the ML code where PropsWithChildren
can be used rather than children?: React.ReactNode
.
But to not hold up this PR I'll approve and I'll fix them in a follow up.
…ges (#182014) ## Summary Original problem: `PropsWithChildren` require a generic type parameter (there's no default). This was not made visible in the merged PR, because we had type-checking on the PRs temporarily (accidentally) removed. Thsi PR fixes the fallout from #181257 => Errors: https://buildkite.com/elastic/kibana-on-merge/builds/44454
Following up on #181257, adding `PropsWithChildren` to the types which were missed.
## Summary Prep work for React@18 bump tl;dr In React@18 `React.FC` doesn't contain `children` anymore, so in order to make the bump easier I have decided to split the effort in multiple faces and hopefully this will make it easier for everyone This PR focuses only on adding explicit `children` declaration either by using `React.PropsWithChildren` type or by adding `children: React.ReactNode` to the existing props types DefinitelyTyped/DefinitelyTyped#46691 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Sergi Massaneda <sergi.massaneda@gmail.com> Co-authored-by: Marco Vettorello <marco.vettorello@elastic.co> Co-authored-by: James Gowdy <jgowdy@elastic.co>
…ges (elastic#182014) ## Summary Original problem: `PropsWithChildren` require a generic type parameter (there's no default). This was not made visible in the merged PR, because we had type-checking on the PRs temporarily (accidentally) removed. Thsi PR fixes the fallout from elastic#181257 => Errors: https://buildkite.com/elastic/kibana-on-merge/builds/44454
Following up on elastic#181257, adding `PropsWithChildren` to the types which were missed.
Summary
Prep work for React@18 bump
tl;dr In React@18
React.FC
doesn't containchildren
anymore, so in order to make the bump easier I have decided to split the effort in multiple faces and hopefully this will make it easier for everyoneThis PR focuses only on adding explicit
children
declaration either by usingReact.PropsWithChildren
type or by addingchildren: React.ReactNode
to the existing props typesDefinitelyTyped/DefinitelyTyped#46691