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

[sharedUX] Move to Package-based Architecture #127546

Merged
merged 16 commits into from
Mar 16, 2022

Conversation

clintandrewhall
Copy link
Contributor

@clintandrewhall clintandrewhall commented Mar 11, 2022

Summary

This was a subset of the work in #127419 for ease of review and commit history. The other three PRs were individually reviewed and then merged into this services PR.

This PR moves portions of the SharedUX plugin out of the plugin and into packages.

Why?

With only a few components, we've run into issues with circular dependencies. The reason being our plugin is consuming code and types from other plugins, and once we do, that plugin can't use our plugin or its components. To mitigate this, we need to keep Shared UX as dependency free as possible.

One of the approaches @majagrubic explored was moving pure components to another plugin to be consumed by dependency plugins. While it worked, we all agreed it wasn't ideal.

Talking with ops, it became clear our assets don't actually need a plugin lifecycle to operate. So I took some time this evening to decouple our components, services and utilities to packages.

Packages

  • @kbn/sharedux-services - Our base service abstraction definitions and mocks.
  • @kbn/sharedux-storybook - All storybook-related code, including a SB-impl of our service layer.
  • @kbn/sharedux-utility - A home for utility code.
  • @kbn/sharedux-components - React components
    • As @spalger has pointed out, this package likely needs to be split apart into smaller packages. At present, we don't have a good sense of how components might be divided, so I'm opting to K.I.S.S. in the immediate term.

Code from src/plugins/shared_ux was refactored and moved to these packages.

Plugin

Now that most of the stateless code has been moved out of the Shared UX plugin, what is its purpose?

The Shared UX plugin provides stateful, Kibana-specific implementations of the services in @kbn/sharedux-services and a React context provider that wires components from @kbn/sharedux-components. Any plugin that is not a dependency for our services can make src/plugins/shared_ux a dependency and consume a pre-wired context that will make components from @kbn/sharedux-components "just work". To make life even easier, the Shared UX plugin re-exports these components, so this becomes an implementation detail.

The nice thing is, dependency plugins that still need to use components from Shared UX but need to avoid the circular dependency can either work with us to hoist the dependency, or consume the pure component from @kbn/sharedux-components, (as we attempted with #126491). I have no doubt there is a better work-around in the medium term, but we need to start somewhere.

I'll flesh out the rest of the details shortly, but I wanted to get this architecture out for feedback before we create more components. I know there's a pressing need to get NoDataView decoupled, so this PR becomes pressing, as well.

How to use services and components after this change

When a consumer of our components and services wants to use them, they have two choices:

  1. use the pre-wired Context from the Shared UX plugin, or
  2. wire it up themselves with their own services value.

Pre-wired Context

The Shared UX plugin imports the SharedUxServicesProvider and populates the services using a servicesFactory that uses Kibana dependencies to fulfill the contract. This is how consumers of the hooks get to the Kibana version of these services: the SharedUxServicesProvider provides the API to those hooks, (and thus the components).

Wiring Manually

You can consider the code in the Shared UX plugin to be a manual wiring of the Provider for components, but Storybook and Jest are also examples:

@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. v8.2.0 Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) labels Mar 11, 2022
@clintandrewhall clintandrewhall requested a review from a team March 11, 2022 21:23
@clintandrewhall clintandrewhall added the backport:skip This commit does not require backporting label Mar 11, 2022
@majagrubic
Copy link
Contributor

majagrubic commented Mar 14, 2022

I didn't mean to "request changes", just wrote some comments 😬 But since it's already there, it can read as "no merging until Maja understands how are the services wired" 😅

@clintandrewhall
Copy link
Contributor Author

@elasticmachine merge upstream

@clintandrewhall clintandrewhall force-pushed the shared_ux/packages/services branch from 15785cb to dddafc0 Compare March 14, 2022 17:39
* plugins-- we have to set this to `unknown`. If and when `DataView` is exported from a
* stateless package, we can remove this.
*/
type DataView = unknown;
Copy link
Contributor

Choose a reason for hiding this comment

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

I accidentally left a comment in the PoC PR, so I'll just leave it here 😅
https://github.com/elastic/kibana/pull/127419/files#r826252775

Copy link
Contributor

Choose a reason for hiding this comment

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

This should really be prioritized imho.

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.

The architectural change looks good. I would just suggest moving that comment to my question to the PR description itself, for easier reference later on.

* [shared-ux][packages] 2. Create Storybook Package

* [shared-ux][packages] 3. Create Utility Package (#127549)

* [shared-ux][packages] 3. Create Utility Package

* [shared-ux][packages] 4. Create Components Package (#127551)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@clintandrewhall clintandrewhall requested review from a team as code owners March 14, 2022 21:25
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.

LGTM, thanks for moving more to packages!

@clintandrewhall clintandrewhall changed the title [shared-ux][packages] 1. Create Services Package [shared-ux] Move to Package-based Architecture Mar 15, 2022
@clintandrewhall clintandrewhall force-pushed the shared_ux/packages/services branch from beda172 to 26b25e3 Compare March 15, 2022 04:56
@clintandrewhall
Copy link
Contributor Author

@elasticmachine merge upstream

@clintandrewhall clintandrewhall changed the title [shared-ux] Move to Package-based Architecture [sharedUX] Move to Package-based Architecture Mar 16, 2022
@clintandrewhall
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default CI Group #2 / Canvas app security feature controls no canvas privileges "before all" hook for "returns a 403"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
sharedUX 61 28 -33

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 - 2 +2
@kbn/shared-ux-services - 43 +43
@kbn/shared-ux-storybook - 2 +2
@kbn/shared-ux-utility - 3 +3
total +50

Async chunks

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

id before after diff
sharedUX 102.9KB 0.0B -102.9KB

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-components - 2 +2
kibana 302 309 +7
sharedUX 1 0 -1
total +8

Page load bundle

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

id before after diff
sharedUX 5.5KB 2.4KB -3.1KB
Unknown metric groups

API count

id before after diff
@kbn/shared-ux-components - 18 +18
@kbn/shared-ux-services - 67 +67
@kbn/shared-ux-storybook - 10 +10
@kbn/shared-ux-utility - 9 +9
sharedUX 16 6 -10
total +94

async chunk count

id before after diff
sharedUX 5 0 -5

ESLint disabled in files

id before after diff
@kbn/shared-ux-components - 2 +2
sharedUX 4 2 -2
total -0

ESLint disabled line counts

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

Total ESLint disabled count

id before after diff
@kbn/shared-ux-components - 5 +5
@kbn/shared-ux-services - 1 +1
@kbn/shared-ux-storybook - 1 +1
sharedUX 7 2 -5
total +2

History

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

@clintandrewhall clintandrewhall enabled auto-merge (squash) March 16, 2022 21:13
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Sass file move LGTM 😉

@clintandrewhall clintandrewhall merged commit 71af73c into main Mar 16, 2022
@clintandrewhall clintandrewhall deleted the shared_ux/packages/services branch March 16, 2022 21:13
@clintandrewhall clintandrewhall self-assigned this Mar 17, 2022
maksimkovalev pushed a commit to maksimkovalev/kibana that referenced this pull request Mar 18, 2022
* [shared-ux][packages] 1. Create Services Package

* Address review feedback

* [shared-ux][packages] 2. Create Storybook Package (elastic#127548)

* [shared-ux][packages] 2. Create Storybook Package

* [shared-ux][packages] 3. Create Utility Package (elastic#127549)

* [shared-ux][packages] 3. Create Utility Package

* [shared-ux][packages] 4. Create Components Package (elastic#127551)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* Merging

* Adding docs

* A few fixes

* Fix TS types

* Fix TS types

* Fix i18n

Co-authored-by: Kibana Machine <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: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.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants