[SharedUX][PoC] Move to a package architecture #127419

clintandrewhall · 2022-03-10T05:37:44Z

Summary

This PR is a proof-of-concept, but is fully-functional. It incorporates concepts from #127415 and #126491 and conversations with folks from @elastic/kibana-operations. I'm publishing early for feedback, so some docs are missing, but will be added if we decide to go this route.

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.

Feedback welcome!

clintandrewhall · 2022-03-10T05:40:07Z

packages/kbn-sharedux-components/src/empty_state/no_data_views/no_data_views.tsx


 import { NoDataViews as NoDataViewsComponent } from './no_data_views.component';

 export interface Props {
-  onDataViewCreated: (dataView: DataView) => void;
+  onDataViewCreated: (dataView: unknown) => void;


I want there to be a better type for this. I've asked in the Typescript group.

What's wrong with DataView?

@majagrubic DataView is a type from src/plugins/data_views/public. Packages cannot depend on plugins.

majagrubic

One overall comments / questions:

Do we need a separate storybook package? Is this to keep components as small as possible? Because it feels this should be a part of the components package
I don't fully understand where are services getting their dependencies from. Where is dataViewEditor being plugged in?

majagrubic · 2022-03-10T10:23:45Z

packages/kbn-sharedux-components/src/empty_state/no_data_views/no_data_views.test.tsx

 import { NoDataViews } from './no_data_views';

 describe('<NoDataViewsPageTest />', () => {
  let services: SharedUXServices;
  let mount: (element: JSX.Element) => ReactWrapper;

  beforeEach(() => {
-    services = servicesFactory();
+    services = mockServiceFactories.servicesFactory();


If the module is called somethingServiceFactories, should the method be called services rather than servicesFactory?

The singleton is a collection of all of the factories, and the servicesFactory universal factory for all of them. I think the naming could definitely use some work.

majagrubic · 2022-03-10T10:25:30Z

packages/kbn-sharedux-services/BUILD.bazel

@@ -0,0 +1,126 @@
+load("@npm//@bazel/typescript:index.bzl", "ts_config")


Is this something auto-generated?

@spalger wrote a script, but we have to edit it in some cases: #127095

majagrubic · 2022-03-10T10:38:21Z

PS. I've never seen an impact label on a PR; thought that was just for issues

clintandrewhall · 2022-03-10T14:52:44Z

Do we need a separate storybook package? Is this to keep components as small as possible? Because it feels this should be a part of the components package

The intention is to eventually split the components package into smaller packages. The catch with packages compared to plugins is you can import code async, but like any package from npm, you get the entire package. For now, our component footprint is small and the divides aren't as clear.

I don't fully understand where are services getting their dependencies from. Where is dataViewEditor being plugged in?

Check out src/plugins/shared_ux/public/services/editors.

clintandrewhall · 2022-03-10T14:56:55Z

@elasticmachine merge upstream

clintandrewhall · 2022-03-10T16:30:01Z

@elasticmachine merge upstream

clintandrewhall · 2022-03-11T05:33:48Z

@elasticmachine merge upstream

kibana-ci · 2022-03-11T06:56:48Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`sharedUX`	49	55	+6

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`	-	1	+1
`@kbn/shared-ux-services`	-	35	+35
`@kbn/shared-ux-storybook`	-	2	+2
total			+38

Async chunks

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

id	before	after	diff
`sharedUX`	100.7KB	100.9KB	+272.0B

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`	-	1	+1
`@kbn/shared-ux-services`	-	1	+1
`kibana`	302	307	+5
`sharedUX`	1	0	-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
`sharedUX`	4.8KB	5.1KB	+298.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/shared-ux-components`	-	12	+12
`@kbn/shared-ux-services`	-	49	+49
`@kbn/shared-ux-storybook`	-	8	+8
`@kbn/shared-ux-utility`	-	4	+4
total			+73

ESLint disabled line counts

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

Total ESLint disabled count

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

History

💔 Build #29708 failed 42f3b33
💔 Build #29706 failed 0810159
💔 Build #29700 failed 5b9777e
💔 Build #29566 failed 8c1b0bd
💔 Build #29499 failed 956a651

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

clintandrewhall · 2022-03-11T22:39:29Z

For ease of review and commit history, this PoC PR has been split into these four stacked pull requests:

Closing out the PR. Thanks all for the feedback and support!

majagrubic · 2022-03-14T18:19:08Z

packages/kbn-shared-ux-components/src/empty_state/no_data_views/no_data_views.tsx


 import { NoDataViews as NoDataViewsComponent } from './no_data_views.component';

 export interface Props {
-  onDataViewCreated: (dataView: DataView) => void;
+  onDataViewCreated: (dataView: unknown) => void;


Since we're in the process of migrating to packages, having a types package is a pretty standard deal. We should really work on getting one for common types, at least the ones from data and data_view plugins. We made all this effort to migrate to typescript, only to be taking a step back now. Is this on your radar as well @spalger ?

I'm not sure if that would accomplish more than colocating the types today. Technically, Bazel already builds a separate types "package"... and I'd be worried about keeping types too far from the relevant code.

At the same time, I'd be interested in creating a @kbn/shared-ux-types package for common and public types. I'd just want to keep it curated to truly universal/common types, rather than default to putting all types there.

It's a good point. Let's think on it a bit, see what we'd propose to put in a types package to see if there'd be value in it. @spalger @stacey-gammon I'd be curious on your input here, as well.

My opinion is to stick to our '"organize by domain" philosophy and co-locate types with the thing they are describing (meaning DataView types should be next to DataView implementation stuff).

Am I understanding the issue here is that packages can't depend on plugins and thats why this had to change from a type to unknown?

I think we will eventually be able to solve this when each plugin is a package, and there is no separate packages directory at all (#112886)

cc @mistic

Yeah, I feel like shared-ux is just a little ahead of the curve here. The DataView plugin will eventually be a package and @kbn/shared-ux* will be able to import the DataView type from it, a @kbn/data-views-types package, or something similar but co-located with the data views implementation stuff.

I'd just want to keep it curated to truly universal/common types, rather than default to putting all types there.

My opinion is to stick to our '"organize by domain" philosophy and co-locate types with the thing they are describing

Totally agree +++

[SharedUX][PoC] Move to a package architecture

dd7c1ad

clintandrewhall requested a review from a team March 10, 2022 05:37

clintandrewhall requested review from a team as code owners March 10, 2022 05:37

clintandrewhall requested review from spalger and majagrubic March 10, 2022 05:38

clintandrewhall commented Mar 10, 2022

View reviewed changes

majagrubic reviewed Mar 10, 2022

View reviewed changes

Updating

9105cf0

kibanamachine and others added 2 commits March 10, 2022 09:56

Merge branch 'main' into sharedux/package-architecture

3acbbe9

Fix ts

757a6c5

clintandrewhall force-pushed the sharedux/package-architecture branch from c2d2706 to 757a6c5 Compare March 10, 2022 15:14

kibanamachine and others added 3 commits March 10, 2022 11:30

Merge branch 'main' into sharedux/package-architecture

0892e62

Fix rename

956a651

Add external type

8c1b0bd

clintandrewhall force-pushed the sharedux/package-architecture branch from 978c447 to 8c1b0bd Compare March 10, 2022 19:03

clintandrewhall added 3 commits March 10, 2022 20:49

Rename packages

5b9777e

Tweak BAZEL and service

0810159

i18n

42f3b33

kibanamachine and others added 2 commits March 11, 2022 00:33

Merge branch 'main' into sharedux/package-architecture

3253b55

i18n snapshots

2222605

clintandrewhall closed this Mar 11, 2022

majagrubic reviewed Mar 14, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SharedUX][PoC] Move to a package architecture #127419

[SharedUX][PoC] Move to a package architecture #127419

clintandrewhall commented Mar 10, 2022

clintandrewhall Mar 10, 2022

majagrubic Mar 10, 2022

clintandrewhall Mar 11, 2022

majagrubic left a comment •

edited

Loading

majagrubic Mar 10, 2022

clintandrewhall Mar 10, 2022

majagrubic Mar 10, 2022

clintandrewhall Mar 10, 2022

majagrubic commented Mar 10, 2022

clintandrewhall commented Mar 10, 2022

clintandrewhall commented Mar 10, 2022

clintandrewhall commented Mar 10, 2022

clintandrewhall commented Mar 11, 2022

kibana-ci commented Mar 11, 2022

API count

ESLint disabled line counts

Total ESLint disabled count

clintandrewhall commented Mar 11, 2022 •

edited

Loading

majagrubic Mar 14, 2022 •

edited

Loading

clintandrewhall Mar 14, 2022

stacey-gammon Mar 14, 2022

spalger Mar 14, 2022 •

edited

Loading

sebelga Mar 15, 2022

		@@ -0,0 +1,126 @@
		load("@npm//@bazel/typescript:index.bzl", "ts_config")

[SharedUX][PoC] Move to a package architecture #127419

[SharedUX][PoC] Move to a package architecture #127419

Conversation

clintandrewhall commented Mar 10, 2022

Summary

Why?

Packages

Plugin

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majagrubic left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majagrubic commented Mar 10, 2022

clintandrewhall commented Mar 10, 2022

clintandrewhall commented Mar 10, 2022

clintandrewhall commented Mar 10, 2022

clintandrewhall commented Mar 11, 2022

kibana-ci commented Mar 11, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

clintandrewhall commented Mar 11, 2022 • edited Loading

majagrubic Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majagrubic left a comment •

edited

Loading

clintandrewhall commented Mar 11, 2022 •

edited

Loading

majagrubic Mar 14, 2022 •

edited

Loading

spalger Mar 14, 2022 •

edited

Loading