[meta] Track plugin dependency workarounds in Shared UX packages

[clintandrewhall]

Summary

Shared UX is moving to a package-first architecture, where code is decoupled from plugin dependencies and placed in stateless packages. The biggest reason for this is to avoid circular dependencies: if any shared component relies on the assets of a plugin, that plugin cannot use any assets provided by a Shared UX plugin without a circular dependency.

You can read more about this in https://github.com/elastic/kibana/pull/127546

Unfortunately, some Shared UX code still relies on plugin assets. Until those assets are hoisted to a package of their own, we've had to apply a few workarounds to make the decoupling complete.

This issue tracks those workarounds until the code is hoisted.

Workarounds

Package	Location	Plugin	Description
kbn-shared-ux-services	/src/services/editors.ts	dataViews	DataView type is not available
kbn-shared-ux-components	/src/redirect_app_links/redirect_app_links.tsx	core	ApplicationStart type is not available

[lukeelmers]

@clintandrewhall Looked at the PR you referenced here -- IIUC the workaround at this point is basically setting these types to unknown (edit: or recreating bits of them)? I worry about the maintenance burden this creates (I don't want to accidentally break things for y'all because TS lets us get away with it), but I realize it is a tradeoff for the benefits you gain from a package-based architecture.

If you are going to be quickly moving in this direction, we'll need to figure out how to prioritize relocating our types. cc @elastic/kibana-core

[clintandrewhall]

@lukeelmers Yep. I realize we're on the forefront here, so we didn't want to burden teams by making them a blocker. I would certainly look into hoisting portions of your APIs that don't directly rely on the plugin lifecycle into packages. At present, dataViewEditor can't rely on the sharedUX plugin due to the circular dependency... but it can use the component out of @kbn/shared-ux-components and provide the API directly.

In terms of the DataView cast to unknown: there just wasn't a way to have the method infer the type, even with generics. It was the fastest way to get it finished. We'll monitor this and, if it becomes a problem, work with teams like core to find an easier way.

[pgayvallet]

The kibana_react plugin (that really never was a plugin, glad we're thinking of moving bits of it to actual stateless modules fwiw), was always made with the assumption that it could access core's types. Most extractions from it to a package will be hardblocked by the fact that some components depends on core types. RedirectAppLinks is not an isolated case, it would be the same for the useKibana react hook, for instance.

we'll need to figure out how to prioritize relocating our types

I gonna be totally honest with everyone here, having done this exercice for both @kbn/logging and @kbn/config, this is a lot of effort, and I'm not sure having some UI components being moved to a package being a sufficient reason to be thinking about moving core types (either all or a subset, both have their own implications too) to their own package. I insist, this is a lot of effort, and also has the side effect of loosing all TSDoc generation associated with those types. This is not something to be taken lightly ihmo. I'm not really opposed to it, but I would really like to have a proper discussion on that to balance the pros and cons.

The workaround, to decouple these components from core's type, is to extract the subsets of signatures the components actually need to live.

If I take the RedirectAppLinks example

export const RedirectAppLinks: FunctionComponent<RedirectCrossAppLinksProps> = ({
  application,
  children,
  className,
  ...otherProps
})

interface RedirectCrossAppLinksProps extends React.HTMLAttributes<HTMLDivElement> {
  application: ApplicationStart;
  className?: string;
  'data-test-subj'?: string;
}

when looking at the component, the only APIs used from ApplicationStart are:

• currentAppId$
• navigateToUrl

When moving the component to a package, I would just extract the required subset of signature in an compatible interface

interface RedirectCrossAppLinksProps extends React.HTMLAttributes<HTMLDivElement> {
--   application: ApplicationStart;
++ application: ApplicationApi;
  className?: string;
  'data-test-subj'?: string;
}

++ interface ApplicationApi {
++   currentAppId$: Observable<string>;
++   navigateToUrl: (url: string) => Promise<void>;
++ }

Now, I agree that if this works for this example because the effectively used APIs from ApplicationStart don't require to import/shim additional types may not work for everything (like, if we want to move useKibana to a package, we're just dead with this approach), so maybe we need a broader vision of exactly what is planned to be extracted to this/these new package(s)?

[clintandrewhall]

@pgayvallet Good points here. I think this boils down to two scenarios, rather than one:

1. Types for data structures that are co-located with components (e.g. DataView) that can result in circular dependencies with Shared UX (and other plugins), and,
2. Types for plugins that shouldn't be extracted and should be reconstituted with the properties necessary for a package-based component or service, (e.g. ApplicationStart)

I'd love to see instances of item 1 moved to packages, and item 2 left as they are, (with the understanding that, if the type upon which the reconstituted type is changed, it will become incompatible with the Shared UX packaged component or service).

Does that make sense?

When moving the component to a package, I would just extract the required subset of signature in an compatible interface

That's what I did with the ApplicationStart type in redirect app links in #127546.

Also, useKibana needs to die. It's bloated and hides away a lot of unused complexity. We're storing entire start contracts in React context and no one knows that's what they're including.

[pgayvallet]

I'd love to see instances of item 1 moved to packages, and item 2 left as they are, (with the understanding that, if the type upon which the reconstituted type is changed, it will become incompatible with the Shared UX packaged component or service). Does that make sense?

It does to me

Also, useKibana needs to die. It's bloated and hides away a lot of unused complexity

Please, do. You're preaching to the choir here :)

[jasonrhodes]

Just to make sure I understand something here, I think I really like the idea of a useKibana hook but in one of these two scenarios:

Caller-only

Shared components require specific dependencies, and callers can use the hook to store/access their start contracts, select the required dependency(ies), and pass them explicitly into the shared component.

// inside myPlugin's own component
import { CoolSharedComponent } from "@kbn/cool-shared";

export MySectionOfPage: React.FC = () => {
  const { ml } = useKibana();

  return (
    <SomeLayout>
      <CoolSharedComponent requiredMLMethodX={ml.methodX} />
   </SomeLayout>
  );
}

Kibana Ecosystem

The more I look at the pattern described above, the more I wonder whether we really need to force all callers to do this dance when the abstraction is already leaked out (you need to depend on the "ML" plugin and then you need to get function x from that plugin and pass it into me in order to use this component).

Perhaps shared components could just somehow specify the plugin dependencies they require (to use this component, you must depend on plugins x and z), and then allow those shared components to access them using useKibana() the way they do now? I'd love to hear what you've discovered about why this pattern causes problems, @clintandrewhall — I'm sure there are lots of things I'm not thinking about, not having done the same work recently. :)

Core-Only Dependencies

Maybe the real answer is that shared components shouldn't rely on plugin dependencies at all. If this were true, using useKibanaCore() inside components would feel even more acceptable to me, rather than forcing callers to have to understand which parts of core a given component relies on (alternatively they could just require you to pass in core but that also seems like extra shuffling...).

I think the only way we get here, though, is by allowing plugins to move almost all of their code out into other static packages so that shared components can import what they need from those places and skip any kind of component -> plugin dependencies whatsoever.

[clintandrewhall]

@jasonrhodes I'm still recovering from strep, but take a look at https://github.com/elastic/kibana/pull/130355 where I landed on a pattern that is similar to what you're describing:

• README for Shared Components: https://github.com/elastic/kibana/blob/main/packages/shared-ux/README.mdx
• Services for ExitFullScreenButton: https://github.com/elastic/kibana/blob/main/packages/shared-ux/button/exit_full_screen/src/services.tsx#L18

You can see that there's an "abstracted" version of the services that pertains specifically to the component, and then a Kibana-based "adapter" that takes a portion of the start contract from CoreStart and populates the abstracted Provider.

What this allows is the ability to pass entire start contracts that are then reduced to only the necessary logic while maintaining a simplified version upon which the component relies. This adds the benefit of components sharing services without relying on CoreStart or plugin contracts to define them, (e.g. setIsFullScreen is common between several components, hoist the service definition and context and populate without relying on the Plugin definition)

Of course, this could all change again, but keeping these contexts abstracted and simple allows us to also control when they might change, preventing re-renders or other side-effects.

I'll have more of an update on this pattern as I recover and migrate more of our components to this model. Feedback welcome!

[clintandrewhall]

@jasonrhodes The biggest drawbacks to the current model include:

• having to mock/implement entire start contract objects when you want to use a component and the contract is unavailable, not desired, etc. Storybook and Jest are great examples of how difficult this is, (e.g. huge portions of coreStart cast as any).
• contexts containing enormous amounts of nested objects. I don't have solid numbers on performance implications, but if even one of those methods updates, the entire tree would be subject to re-render.
• lack of solid/consistent understanding of dependencies-- calling useKibana to simply find if Kibana is in light or dark mode. The abstraction is massive and deceptively "simple".
• among others I'm sure I'm not remembering.

I'm sure you agree, hooks/contexts/etc should be simple, even single purpose. Having a hook that returns a (or several) massive contracts when your application may only need one or two methods of functionality seems wasteful. I still need to do more research on measurement, though.

[lukasolson]

Can this be closed now that types can now be imported inside packages from plugins?