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

[Shared UX][packages] Split up dependencies; add Storybook mock #136488

Merged
merged 16 commits into from
Jul 28, 2022

Conversation

clintandrewhall
Copy link
Contributor

@clintandrewhall clintandrewhall commented Jul 16, 2022

Summary

Fixes #136247

This is a large PR, but it's mostly moving code out into packages. Due to the dependency tree, splitting this up would mean a lot of half-measures leading to this point, and more time to complete.

This PR splits types and mocks from each component package into their own packages. It also leverages a new Storybook mock manager to make composing stories standard (and easier).

TODO

  • READMEs
  • Documentation
  • Add tests

Why?

Jest tests and Storybook stories live alongside their subjects. This is fine when testing a single component in its own package, but when composing components in a parent, the parent package needs access to the mocks in the composed components. This means the composed component packages need to export their mocks. And that means the mocks (and all of their dependencies) become part of the bundle.

In #136245 I explored splitting things up to eliminate the dependencies. This PR uses the peer feedback and applies that proof-of-concept into a hardened approach.

How?

  • The /[package] directory becomes a folder, rather than the package.
  • Source code for the package is moved to [package]/impl.
  • All mocks are moved to [package]/mocks.
  • All types for a package are moved to [package]/types to be used in both [package]/impl and [package]/mocks.
  • [package]/mocks packages are then left out of the BUILD.bazel files for dependent packages.
  • Service and Prop types are re-exported from [package] for ease-of-use for consumers.

Next up

  • Complete migration of NoDataPage.
  • Kill shared-ux-services.
  • Kill shared-ux-components.
  • Move shared-ux-storybook.
  • Move shared-ux-utility.

@clintandrewhall clintandrewhall added review loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes impact:critical This issue should be addressed immediately due to a critical level of impact on the product. backport:skip This commit does not require backporting Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.4.0 labels Jul 16, 2022
@clintandrewhall clintandrewhall requested a review from a team July 16, 2022 20:23
@clintandrewhall clintandrewhall force-pushed the packages/split branch 3 times, most recently from c1f1086 to 14eff63 Compare July 18, 2022 00:17
Copy link
Contributor

@sebelga sebelga left a comment

Choose a reason for hiding this comment

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

Overall it looks good to me 👍 I left a few comments about naming convention and import.

If we could stick to import every import type from the _types package that would be great. Unless we would then have circular dependencies?

I imagine that once this architecture is in place we will have the tooling to create a package that will

  • auto generate the 3 folders for each of the internal packages.
  • declare the package in the main BAZEL file
  • update the package.json

Is that the plan?

@clintandrewhall clintandrewhall self-assigned this Jul 26, 2022
@clintandrewhall clintandrewhall requested a review from a team as a code owner July 27, 2022 02:19
@clintandrewhall clintandrewhall enabled auto-merge (squash) July 28, 2022 15:55
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dashboard 423 377 -46
dataViewManagement 176 175 -1
discover 601 555 -46
home 198 197 -1
kibanaOverview 170 124 -46
lens 943 897 -46
observability 475 474 -1
securitySolution 3237 3236 -1
visualizations 364 318 -46
total -234

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/shared-ux-button-exit-full-screen-mocks - 9 +9
@kbn/shared-ux-card-no-data 8 4 -4
@kbn/shared-ux-card-no-data-mocks - 28 +28
@kbn/shared-ux-link-redirect-app-mocks - 7 +7
@kbn/shared-ux-page-analytics-no-data 8 4 -4
@kbn/shared-ux-page-analytics-no-data-mocks - 12 +12
@kbn/shared-ux-page-kibana-no-data 12 3 -9
@kbn/shared-ux-page-kibana-no-data-mocks - 24 +24
@kbn/shared-ux-prompt-no-data-views 13 4 -9
@kbn/shared-ux-prompt-no-data-views-mocks - 16 +16
@kbn/shared-ux-storybook-mock - 4 +4
total +74

Async chunks

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

id before after diff
dashboard 409.7KB 372.3KB -37.5KB
dataViewManagement 143.6KB 143.6KB -3.0B
discover 501.1KB 463.7KB -37.5KB
kibanaOverview 82.0KB 44.5KB -37.5KB
lens 1.2MB 1.2MB -37.5KB
maps 2.6MB 2.6MB +17.0B
visualizations 237.1KB 199.2KB -37.9KB
total -187.8KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/shared-ux-button-exit-full-screen-types - 3 +3
@kbn/shared-ux-card-no-data 1 0 -1
@kbn/shared-ux-card-no-data-types - 5 +5
@kbn/shared-ux-link-redirect-app-types - 3 +3
@kbn/shared-ux-page-analytics-no-data 5 1 -4
@kbn/shared-ux-page-analytics-no-data-types - 5 +5
@kbn/shared-ux-page-kibana-no-data 2 0 -2
@kbn/shared-ux-page-kibana-no-data-types - 4 +4
@kbn/shared-ux-prompt-no-data-views 2 0 -2
@kbn/shared-ux-prompt-no-data-views-types - 5 +5
@kbn/shared-ux-storybook-mock - 1 +1
total +17

Page load bundle

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

id before after diff
cloudSecurityPosture 8.7KB 8.6KB -23.0B
kibanaOverview 15.3KB 15.3KB -1.0B
security 51.4KB 51.4KB -23.0B
visualizations 46.8KB 46.8KB -1.0B
total -48.0B
Unknown metric groups

API count

id before after diff
@kbn/shared-ux-button-exit-full-screen-mocks - 13 +13
@kbn/shared-ux-card-no-data 15 10 -5
@kbn/shared-ux-card-no-data-mocks - 32 +32
@kbn/shared-ux-link-redirect-app-mocks - 8 +8
@kbn/shared-ux-page-analytics-no-data 12 13 +1
@kbn/shared-ux-page-analytics-no-data-mocks - 12 +12
@kbn/shared-ux-page-kibana-no-data 31 26 -5
@kbn/shared-ux-page-kibana-no-data-mocks - 25 +25
@kbn/shared-ux-prompt-no-data-views 22 24 +2
@kbn/shared-ux-prompt-no-data-views-mocks - 17 +17
@kbn/shared-ux-storybook-mock - 14 +14
total +114

History

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

cc @clintandrewhall

@clintandrewhall clintandrewhall merged commit 3f7f972 into elastic:main Jul 28, 2022
@clintandrewhall clintandrewhall deleted the packages/split branch July 28, 2022 17:00
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 impact:critical This issue should be addressed immediately due to a critical level of impact on the product. loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes review Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[discuss] Sharing Storybook/Jest mocks between packages
5 participants