Skip to content

Add explicit children types#181257

Merged
patrykkopycinski merged 14 commits intoelastic:mainfrom
patrykkopycinski:chore/react-18-props
Apr 29, 2024
Merged

Add explicit children types#181257
patrykkopycinski merged 14 commits intoelastic:mainfrom
patrykkopycinski:chore/react-18-props

Conversation

@patrykkopycinski
Copy link
Copy Markdown
Contributor

@patrykkopycinski patrykkopycinski commented Apr 21, 2024 •
edited
Loading

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

@patrykkopycinski
Copy link
Copy Markdown
Contributor Author

patrykkopycinski commented Apr 21, 2024

/ci

@patrykkopycinski
Copy link
Copy Markdown
Contributor Author

patrykkopycinski commented Apr 21, 2024

/ci

@patrykkopycinski patrykkopycinski self-assigned this Apr 21, 2024
@patrykkopycinski patrykkopycinski marked this pull request as ready for review April 22, 2024 13:59
@patrykkopycinski patrykkopycinski requested review from a team as code owners April 22, 2024 13:59
@patrykkopycinski patrykkopycinski requested a review from a team April 22, 2024 13:59
@patrykkopycinski patrykkopycinski requested review from a team as code owners April 22, 2024 13:59
@patrykkopycinski patrykkopycinski requested a review from a team April 22, 2024 13:59
@patrykkopycinski patrykkopycinski requested review from a team as code owners April 22, 2024 13:59
@patrykkopycinski patrykkopycinski requested a review from a team April 22, 2024 13:59
@patrykkopycinski patrykkopycinski requested a review from a team as a code owner April 22, 2024 13:59
@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Apr 29, 2024
@patrykkopycinski
Copy link
Copy Markdown
Contributor Author

patrykkopycinski commented Apr 29, 2024

Thank you for the feedback @jgowdyelastic @cnasikas @nikitaindik , I have aligned all the changes to use PropsWithChildren

cnasikas
cnasikas approved these changes Apr 29, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

ResponseOps changes LGTM!

patrykkopycinski and others added 3 commits April 29, 2024 14:02
@patrykkopycinski @jgowdyelastic
Update x-pack/plugins/ml/public/application/data_frame_analytics/page…
eae8afc 
…s/analytics_creation/components/details_step/description.tsx

Co-authored-by: James Gowdy <jgowdy@elastic.co>
@patrykkopycinski
fix
8112b26
@patrykkopycinski
Merge branch 'chore/react-18-props' of github.com:patrykkopycinski/ki…
d4f5d2b 
…bana into chore/react-18-props

# Conflicts:
#	x-pack/plugins/ml/public/application/data_frame_analytics/pages/analytics_creation/components/details_step/description.tsx
@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Apr 29, 2024 •
edited
Loading

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/cell-actions 46 44 -2
@kbn/react-kibana-mount 7 8 +1
@kbn/search-api-panels 81 82 +1
spaces 65 66 +1
total +1

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id before after diff
@kbn/i18n-react 0 1 +1
@kbn/ml-url-state 1 2 +1
esUiShared 3 4 +1
presentationUtil 2 3 +1
total +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
enterpriseSearch 2.7MB 2.7MB +77.0B
uiActionsEnhanced 135.7KB 135.7KB +10.0B
total +87.0B

Canvas Sharable Runtime

The Canvas "shareable runtime" is an bundle produced to enable running Canvas workpads outside of Kibana. This bundle is included in third-party webpages that embed canvas and therefor should be as slim as possible.

id before after diff
module count - 5874 +5874
total size - 6.7MB +6.7MB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
infra 102.5KB 102.5KB +13.0B
kibanaReact 40.3KB 40.3KB +4.0B
spaces 25.6KB 25.6KB +11.0B
total +28.0B
Unknown metric groups

API count

id before after diff
@kbn/cell-actions 64 56 -8
@kbn/react-kibana-mount 10 11 +1
@kbn/search-api-panels 81 82 +1
spaces 256 257 +1
total -5

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @patrykkopycinski

@patrykkopycinski patrykkopycinski enabled auto-merge (squash) April 29, 2024 14:06
jgowdyelastic
jgowdyelastic approved these changes Apr 29, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@jgowdyelastic jgowdyelastic left a comment •
edited
Loading

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.

@patrykkopycinski patrykkopycinski merged commit 0780c19 into elastic:main Apr 29, 2024
@kibanamachine kibanamachine added v8.15.0 backport:skip This PR does not require backporting labels Apr 29, 2024
@jgowdyelastic jgowdyelastic mentioned this pull request Apr 29, 2024
Merged
@delanni delanni mentioned this pull request Apr 29, 2024
Merged
mistic pushed a commit that referenced this pull request Apr 29, 2024
@delanni
[Fix] fix type issues from unparameterized PropsWithChildren type usa…
4a90df2 
…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
jgowdyelastic added a commit that referenced this pull request Apr 30, 2024
@jgowdyelastic
[ML] Using PropsWithChildren type (#182008)
6023164 
Following up on #181257, adding `PropsWithChildren` to the types which
were missed.
@kibanamachine kibanamachine mentioned this pull request May 23, 2024
Merged
@kibanamachine kibanamachine mentioned this pull request Jul 4, 2024
Merged
@delanni
Copy link
Copy Markdown
Member

delanni commented Aug 6, 2024

@patrykkopycinski - this PR is incorrectly marked with the label v8.14.0, and it wasn't in fact backported to 8.14.
If this is a preparation for React 18, maybe this shouldn't be backported to 8.14? Could you suggest if this is needed or not?

I've a related PR that patched a few things after this (#182014) and that's also pending backports to 8.14. If this one will not be backported, my PR should also not be.

@delanni
Copy link
Copy Markdown
Member

delanni commented Aug 9, 2024

The react 18 bump will not make it to 8.14, so this change probably won't need to be backported to 8.14. So I'll remove the incorrect labeling on this, and on my fix PR as well.

@delanni delanni removed the v8.14.0 label Aug 9, 2024
@Dosant Dosant mentioned this pull request Sep 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nreese nreese nreese approved these changes

@dej611 dej611 dej611 approved these changes

@jughosta jughosta jughosta approved these changes

@flash1293 flash1293 flash1293 approved these changes

@pgayvallet pgayvallet pgayvallet approved these changes

@e40pud e40pud e40pud approved these changes

@shahzad31 shahzad31 shahzad31 approved these changes

@kpollich kpollich kpollich approved these changes

@stephmilovic stephmilovic stephmilovic approved these changes

@cnasikas cnasikas cnasikas approved these changes

@eokoneyo eokoneyo eokoneyo approved these changes

@jloleysens jloleysens jloleysens approved these changes

@joeypoon joeypoon joeypoon approved these changes

@nikitaindik nikitaindik nikitaindik approved these changes

@PhilippeOberti PhilippeOberti PhilippeOberti approved these changes

@semd semd semd approved these changes

@nickofthyme nickofthyme nickofthyme approved these changes

@opauloh opauloh opauloh approved these changes

@jgowdyelastic jgowdyelastic jgowdyelastic approved these changes

@cauemarcondes cauemarcondes cauemarcondes approved these changes

@sphilipse sphilipse sphilipse approved these changes

@jeramysoucy jeramysoucy jeramysoucy approved these changes

@szwarckonrad szwarckonrad Awaiting requested review from szwarckonrad szwarckonrad was automatically assigned from elastic/security-defend-workflows

+2 more reviewers

@dgieselaar dgieselaar dgieselaar approved these changes

@yuliacech yuliacech yuliacech approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@patrykkopycinski patrykkopycinski

Labels

apm:review backport:skip This PR does not require backporting ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team Team:obs-ux-infra_services - DEPRECATED DEPRECATED - Use Team:obs-presentation. v8.15.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.