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

[kibana_react] Extract <FieldButton /> and <FieldIcon/> to a package #115377

Merged
merged 17 commits into from
Nov 8, 2021

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Oct 18, 2021

Part of #114990

We would like to clean up kibana_react and kibana_utils by moving some of shared stateless code that isn't tightly coupled to core's APIs into packages. This will not only allow us to declutter these plugins which became a place for putting code that is shared between plugins, but will also allow us to reduce initial bundle size because code from packages can be imported in async chunks only! (read on packages vs plugins).

This is an initial "example" pr which moves <FieldButton /> and <FieldIcon/> to a package. It also resolves some missing pieces we had that would allow having a react component with scss in a @kbn/ package (thanks to operations team Dosant#6)
This will serve as a blueprint for moving other components.

p.s. initially, I started with KibanaPageTemplate component but decided to hold off and keep it in a plugin: #114990 (comment)

@Dosant
Copy link
Contributor Author

Dosant commented Oct 19, 2021

@elasticmachine merge upstream

@Dosant
Copy link
Contributor Author

Dosant commented Oct 20, 2021

@elasticmachine merge upstream

@Dosant
Copy link
Contributor Author

Dosant commented Oct 20, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 400 415 +15
graph 154 158 +4
kibanaReact 354 341 -13
lens 834 849 +15
maps 839 843 +4
osquery 239 243 +4
securitySolution 2769 2771 +2
stackAlerts 50 55 +5
total +36

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/react-field - 10 +10
kibanaReact 245 204 -41
total -31

Async chunks

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

id before after diff
discover 332.9KB 343.9KB +11.0KB
graph 403.3KB 404.7KB +1.4KB
lens 1.0MB 1.0MB +9.4KB
maps 2.7MB 2.7MB +1.3KB
osquery 866.3KB 870.6KB +4.3KB
securitySolution 4.6MB 4.6MB +882.0B
stackAlerts 159.0KB 160.7KB +1.7KB
total +29.9KB

Page load bundle

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

id before after diff
kibanaReact 80.9KB 72.2KB -8.7KB
osquery 7.5KB 7.5KB +54.0B
stackAlerts 12.0KB 12.1KB +54.0B
total -8.6KB
Unknown metric groups

API count

id before after diff
@kbn/react-field - 21 +21
kibanaReact 282 228 -54
total -33

History

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

@Dosant Dosant added Feature:kibana-react performance release_note:skip Skip the PR/issue when compiling release notes Team:AppServicesUx technical debt Improvement of the software architecture and operational architecture v8.0.0 labels Oct 21, 2021
@Dosant Dosant marked this pull request as ready for review October 21, 2021 10:05
@Dosant Dosant requested review from a team as code owners October 21, 2021 10:05
Copy link
Contributor

@patrykkopycinski patrykkopycinski left a comment

Choose a reason for hiding this comment

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

Asset management LGTM

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

@stratoula
Copy link
Contributor

@Dosant I see that it fails on yarn kbn bootstrap with this failure
image

@Dosant
Copy link
Contributor Author

Dosant commented Oct 25, 2021

@stratoula, thanks, looks like new usage of a component with merging main, I'll fix

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

VisEditors code changes LGTM. I tested Lens and everything seems to work as expected :)

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

Operations LGTM

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Great work @Dosant , code changes LGTM. Happy to merge with passing CI 👍🏻

packages/kbn-react-field/README.md Outdated Show resolved Hide resolved
Dosant and others added 2 commits October 27, 2021 11:44
…scss-in-packages

# Conflicts:
#	src/plugins/presentation_util/public/components/field_picker/field_picker.tsx
@Dosant Dosant added v8.1.0 and removed v8.0.0 labels Oct 28, 2021
@Dosant
Copy link
Contributor Author

Dosant commented Oct 28, 2021

@elasticmachine merge upstream

@Dosant
Copy link
Contributor Author

Dosant commented Oct 29, 2021

@elasticmachine merge upstream

@Dosant
Copy link
Contributor Author

Dosant commented Oct 30, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 345 355 +10
graph 148 151 +3
kibanaReact 307 298 -9
lens 672 682 +10
maps 797 800 +3
osquery 238 241 +3
presentationUtil 263 273 +10
securitySolution 2752 2754 +2
stackAlerts 45 48 +3
total +35

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/react-field - 10 +10
kibanaReact 245 204 -41
total -31

Async chunks

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

id before after diff
discover 303.3KB 310.7KB +7.4KB
graph 392.2KB 393.4KB +1.2KB
kibanaReact 206.7KB 206.7KB -10.0B
lens 947.7KB 953.7KB +6.0KB
maps 2.6MB 2.6MB +1.2KB
osquery 934.9KB 938.7KB +3.8KB
presentationUtil 210.1KB 210.1KB +12.0B
securitySolution 4.5MB 4.5MB +879.0B
stackAlerts 158.4KB 159.7KB +1.3KB
total +21.6KB

Page load bundle

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

id before after diff
kibanaReact 62.8KB 57.3KB -5.5KB
osquery 6.8KB 6.9KB +54.0B
presentationUtil 62.5KB 68.4KB +5.9KB
stackAlerts 11.9KB 11.9KB +54.0B
total +578.0B
Unknown metric groups

API count

id before after diff
@kbn/react-field - 21 +21
kibanaReact 282 228 -54
total -33

History

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

@Dosant Dosant merged commit bc56e60 into elastic:main Nov 8, 2021
@Dosant
Copy link
Contributor Author

Dosant commented Nov 8, 2021

This will serve as an example of react components in a package for #114990 and others, but unfortunately, no initial bundle size improvement for this one at all because since recently @elastic/kibana-presentation started importing these components to presentation_utils plugins forcing them into Kibana's initial bundle

@elastic/kibana-presentation, fyi, you can read up on plugins vs packges here to see if you it would make sense to move some of presentation_utils code into a package similar to kibana_react

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 10, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 115377 or prevent reminders by adding the backport:skip label.

@Dosant Dosant added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:kibana-react performance release_note:skip Skip the PR/issue when compiling release notes technical debt Improvement of the software architecture and operational architecture v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants