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] Complete package migration; delete plugin #138962

Merged

Conversation

clintandrewhall
Copy link
Contributor

My apologies for the large PR that asks for so many reviews. I was trying to split this up into two PRs, and found the mitigation was actually harder than just the migration. Most teams will see a change in import path only. Your patience is appreciated!

Summary

This PR is large, but completes the migration of Shared UX components to the new package architecture in Kibana.

What does this PR do?

  • Splits NoDataPage, NoDataConfigPage and KibanaPageTemplate into separate packages.
    • Each consists of their impl, mocks and types packages.
  • Deletes the bottleneck @kbn/shared-ux-components and @kbn/shared-ux-services packages.
  • Eliminates the @kbn/shared-ux-storybook package in favor of a local configuration: /packages/shared-ux/.storybook.
  • Eliminates the legacy services architecture.
  • Eliminates the need for the sharedUX plugin and deletes it.
  • Fixes incorrect deprecation warnings and other documentation.

What remains

You may see some README files with boilerplate content. In my next PR, I'm reviewing and adding documentation to all of our component packages. I'm also adding more tests using the new mocks. Check out the Shared UX Project Roadmap for more details, or reach out to me directly.

@clintandrewhall clintandrewhall added review loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. backport:skip This commit does not require backporting Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.5.0 labels Aug 16, 2022
@clintandrewhall clintandrewhall requested a review from a team August 16, 2022 23:13
@clintandrewhall clintandrewhall requested review from a team as code owners August 16, 2022 23:13
@botelastic botelastic bot added Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Aug 16, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@clintandrewhall clintandrewhall requested a review from a team as a code owner August 16, 2022 23:32
@clintandrewhall clintandrewhall requested a review from a team August 16, 2022 23:32
@clintandrewhall clintandrewhall requested review from a team as code owners August 16, 2022 23:32
@clintandrewhall clintandrewhall force-pushed the packages/no_data_page_split branch from d8d94a3 to 18c6233 Compare August 16, 2022 23:43
@clintandrewhall
Copy link
Contributor Author

Sorry for the widespread pings, I rebased and ended up integrating the commits, rather than rebasing upon them. Apologies, should be all set now!

@clintandrewhall clintandrewhall added the Team:Observability Team label for Observability Team (for things that are handled across all of observability) label Aug 16, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/unified-observability (Team:Observability)

@clintandrewhall clintandrewhall force-pushed the packages/no_data_page_split branch from 6a6bc23 to 1b96f68 Compare August 17, 2022 02:01
@clintandrewhall clintandrewhall force-pushed the packages/no_data_page_split branch from 1b96f68 to 7261e65 Compare August 17, 2022 02:30
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 changes LGTM! So glad we've made it here

Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Code review only, let me know if there is a certain part here that needs to be actually tested.

@@ -67,8 +67,6 @@
"savedObjectsManagement": "src/plugins/saved_objects_management",
"server": "src/legacy/server",
"share": "src/plugins/share",
"sharedUX": "src/plugins/shared_ux",
"sharedUXComponents": "packages/kbn-shared-ux-components/src",
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

getRedirectAppLinksKibanaDependenciesMock()
getRedirectAppLinksKibanaDependenciesMock(),
{
isMergeableObject: isPlainObject,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deepmerge merges objects, so jest.fn() objects were also getting merged, which meant they were no longer mocked functions. isMergeableObject allows one to create a function to test if the object should be merged or not. In this case, I'm saying deepmerge should only merge the object if it passes lodash.isPlainObject.

const { hasESData, hasUserDataView } = services;

return (
<KibanaNoDataPageContext.Provider value={{ hasESData, hasUserDataView }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to explicitly destructure services here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

services could include a lot of other things, as TS types do not prevent passing objects with more than is necessary. By destructuring first, we exclude anything extra that was passed, and keep the context value from being unintentionally bloated.

<span>Second Item</span>,
];

export class StorybookMock extends AbstractStorybookMock<
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a bit overly complicated. Do we need this AbstractStorybookMock?

Copy link
Contributor Author

@clintandrewhall clintandrewhall Aug 17, 2022

Choose a reason for hiding this comment

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

I needed a way to make sure that all Storybook mocks followed the same API. This allows us to compose mocked values for services without having to know what services consumed components need.

I grant you: it's complicated. But it's working at the moment. We'll probably refactor it at some point, but I had to start somewhere.

Copy link
Member

@maryam-saeidi maryam-saeidi left a comment

Choose a reason for hiding this comment

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

Code review only, Actionable Observability import changes LGTM.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

kibana-gis changes LGTM
code review

@clintandrewhall clintandrewhall force-pushed the packages/no_data_page_split branch from 8a53424 to a99027e Compare August 17, 2022 17:59
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Import path change to the getting_started page in the home plugin LGTM

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

apm changes lgtm

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
dashboard 384 361 -23
discover 548 525 -23
home 197 179 -18
kibanaOverview 124 105 -19
lens 914 891 -23
observability 474 456 -18
securitySolution 3037 3019 -18
sharedUX 12 - -12
visualizations 318 295 -23
total -177

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-components 6 - -6
@kbn/shared-ux-link-redirect-app-mocks 7 9 +2
@kbn/shared-ux-page-kibana-template - 4 +4
@kbn/shared-ux-page-kibana-template-mocks - 43 +43
@kbn/shared-ux-page-no-data - 4 +4
@kbn/shared-ux-page-no-data-config - 5 +5
@kbn/shared-ux-page-no-data-config-mocks - 24 +24
@kbn/shared-ux-page-no-data-mocks - 26 +26
@kbn/shared-ux-services 51 - -51
@kbn/shared-ux-storybook 7 - -7
observability 396 395 -1
total +43

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/shared-ux-page-kibana-template-mocks - 5 +5

Async chunks

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

id before after diff
dashboard 377.1KB 373.7KB -3.3KB
discover 464.4KB 461.1KB -3.3KB
home 144.2KB 146.8KB +2.6KB
kibanaOverview 44.5KB 46.3KB +1.8KB
lens 1.2MB 1.2MB -2.4KB
observability 504.3KB 506.8KB +2.5KB
securitySolution 5.4MB 5.4MB +2.4KB
visualizations 199.7KB 197.3KB -2.4KB
total -2.2KB

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-page-kibana-no-data-types 4 5 +1
@kbn/shared-ux-page-kibana-template-mocks - 2 +2
@kbn/shared-ux-page-kibana-template-types - 1 +1
@kbn/shared-ux-page-no-data-config-types - 1 +1
@kbn/shared-ux-page-no-data-types - 2 +2
@kbn/shared-ux-services 1 - -1
total +6

Page load bundle

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

id before after diff
home 10.5KB 10.4KB -48.0B
observability 64.8KB 64.8KB -5.0B
sharedUX 2.1KB - -2.1KB
total -2.2KB
Unknown metric groups

API count

id before after diff
@kbn/shared-ux-components 11 - -11
@kbn/shared-ux-link-redirect-app-mocks 8 10 +2
@kbn/shared-ux-page-kibana-no-data 26 8 -18
@kbn/shared-ux-page-kibana-template - 10 +10
@kbn/shared-ux-page-kibana-template-mocks - 43 +43
@kbn/shared-ux-page-no-data - 13 +13
@kbn/shared-ux-page-no-data-config - 11 +11
@kbn/shared-ux-page-no-data-config-mocks - 24 +24
@kbn/shared-ux-page-no-data-mocks - 27 +27
@kbn/shared-ux-services 80 - -80
@kbn/shared-ux-storybook 16 - -16
observability 399 398 -1
sharedUX 4 - -4
total -0

ESLint disabled in files

id before after diff
@kbn/shared-ux-storybook-config - 1 +1
sharedUX 2 - -2
total -1

ESLint disabled line counts

id before after diff
@kbn/shared-ux-services 3 - -3

Total ESLint disabled count

id before after diff
@kbn/shared-ux-services 3 - -3
@kbn/shared-ux-storybook-config - 1 +1
sharedUX 2 - -2
total -4

History

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

Copy link
Contributor

@formgeist formgeist left a comment

Choose a reason for hiding this comment

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

I trust others have reviewed the code - just a designers approval here 😊

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

Security Solution changes LGTM!

@clintandrewhall
Copy link
Contributor Author

Thank you all!

@clintandrewhall clintandrewhall merged commit 0fbd0af into elastic:main Aug 18, 2022
@clintandrewhall clintandrewhall self-assigned this Aug 23, 2022
Mpdreamz pushed a commit to Mpdreamz/kibana that referenced this pull request Sep 6, 2022
* [Shared UX] Complete package migration; delete plugin

* [CI] Auto-commit changed files from 'node scripts/generate packages_build_manifest'

* Fix types, fix tests

* Create Storybook config package; organize Storybook

* [CI] Auto-commit changed files from 'node scripts/build_plugin_list_docs'

* Revise Storybook config package; make mock packages compatible with web

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
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:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes review Team:APM All issues that need APM UI Team support Team:Observability Team label for Observability Team (for things that are handled across all of observability) Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.