[DISCUSS] Registries

[mattkime]

Registries provide a method of augmenting functionality in Kibana. In the new platform registries will be exposed via plugins rather than the ui directory, where most registries currently live. The uiRegistry function generates registry instances with angular dependency injection and IndexedArray functionality, the former is being removed out of necessity and the later for simplicity.

What should replace uiRegistry?

My research into registries indicates that they're simply arrays with accessors. The accessors may be as simple as returning the whole array or provide some sort of search or sort or both. Even the most complex of case can be expressed in 2-3 lines of code.

That said, there is some desire to provide a uniform interface. Perhaps it could be implemented with a class or a typescript interface. I think this is mostly about communication hence my desire get opinions.

Relevant code -
https://github.com/elastic/kibana/blob/master/src/legacy/ui/public/registry/_registry.js
https://github.com/elastic/kibana/blob/master/src/legacy/ui/public/indexed_array/indexed_array.js

@timroes @stacey-gammon @lizozom @lukeelmers @streamich @ppisljar

[elasticmachine]

Pinging @elastic/kibana-app-arch

[lukeelmers]

My two cents: I don't have any strong feelings on the exact interface, mostly I just think we should keep things minimal & avoid overdoing it. In other words, I'm not even certain we need to replace uiRegistry fully.

I agree a lightweight class / TS interface could be one way to accomplish this. I see the benefits of that approach mostly being around developer experience for folks building apps in Kibana... would be nice if any registries they interacted with behaved similarly. But that's primarily a question of convention: it doesn't necessarily require a full registry-building "service" to be implemented.

I know @timroes has put a lot of thought into this; would be interested to hear this opinion.

[stacey-gammon]

mostly I just think we should keep things minimal & avoid overdoing it.

+++

I agree a lightweight class / TS interface could be one way to accomplish this

++

@lizozom and I discussed this at GAH. The benefit of having a base interface is simply just like you say @lukeelmers - convention. e.g. do you call registry.insert or registry.add. An interface can be a nice lightweight way to enforce this consistency without a heavy hand.

[streamich]

This is how I imagine this, there would be some interface that anyone can use (in functional or OOP way — up to you):

export interface Registry<T extends {}> {
  get: (id: string) => T | undefined;
  set: (id: string, obj: T) => void;
  set$: Observable<T>;
  reset: () => void;
  reset$: Observable<void>;
}

(This Registry interface is probably incomplete — missing some useful methods, of course we can add them.)

But you don't have to implement the Registry interface yourself, there would be a factory createRegistry() function that does it for you:

import {Observable, Subject} from 'rxjs';

export interface Registry<T extends {}> {
  get: (id: string) => T | undefined;
  set: (id: string, obj: T) => void;
  set$: Observable<T>;
  reset: () => void;
  reset$: Observable<void>;
}

export const createRegistry = <T>(): Registry<T> => {
  let data = new Map<string, T>();

  const set$ = new Subject();
  const reset$ = new Subject();

  const get = (id: string) => data.get(id);
  const set = (id: string, obj: T) => {
    data.set(id, obj);
    set$.next(obj);
  };
  const reset = () => {
    data = new Map<string, T>();
    reset$.next();
  };

  return {
    get,
    set,
    set$: set$ as any as Observable<T>,
    reset,
    reset$: reset$ as any as Observable<void>,
  }
}

Then new platform plugins would "uiExport" their registries for all other plugins to use:

// NP setup life-cycle
function setup () {
  const visualizationRegistry = createRegistry();

  return {
    visualizationRegistry,
  };
}

One thing the createRegistry() would also need to support is indexing/filtering for registries that have thousand+ entries and where entries are not simply selected by an ID, but by some other fields.

[mattkime]

@streamich I'm curious if we have existing use cases that could take advantage of the observable interface.

[stacey-gammon]

hm, yea good point @mattkime. Until there is a need, why bother including it? Right now I don't think there are any needs for it. Most of the time registries aren't added to dynamically at run time, though with the new actions stuff, they will be. Still don't think we'll need observables for it.

[streamich]

@mattkime @stacey-gammon I'm not aware of any existing registry use cases that would benefit from observables; I was thinking some UI component could use that to re-render when registry state changes and non-UI things too—like telemetry could track something. But, yeah, we should not overcomplicate, it is just an example.

[timroes]

Since @lukeelmers wanted my two cents, here they are:

I think we have a couple of use-cases in the uiRegistry why I in general favor for having a replacement solution (pure TS) version for it. The large two use-cases why I would not want to rebuild it all over the place (and currently missing from the suggested API) are the following:

• Sorting by a specific property (e.g. inspector views are sorted by some order: number, while vis types are sorted by name: string). That way the user can always get the list in the sorted version and doesn't need to take care at the place of consuming of sorting them. This is currently done via the order property passed when configuring your uiRegistry.
• Grouping by a specific key (currently the group config). This allows to group by a specific property e.g. category so you would currently have something like registry.byCategory.misc: T[] where misc might be one value of the category property, now containing an array of all registered items having that key. We should obviously if we need that again not use that not typeable interface, but instead something like registry.by('category').misc instead, which would be properly typeable. We don't use that grouping in too many spaces anymore (feature catalog, navbar extensions), why I think it's discussable if we need that again (in contrast to the ordering functionality, which we use in nearly all the places).

With regards to Observable or not: I think we currently have both use-cases. The problem is, that due to the way Angular initializes things in two phases (config and run phase) we can in most places assume that in the run phase every registry is already settled and not updating anymore. But I already came to issues in PoCs where visualizations needed to register dynamically and in that case I needed some rather nasty workaround for uiRegistry not supporting async registration.

Since we have a similar concept in the new platform of a setup and start method, we hopefully could use "sync" registries in most of the places allowing to register only in setup and thus no need for observables. For that API I actually would prefer if we have have a clear: now we're in registeration mode having add method, etc, vs now we "freeze" the registry and returning a new object not having those manipulating options anymore. That way we would already express that frozen state via types and thus have it way easier checkable.

But there might also be cases where we might want to allow users to register items later on, and in this case we imho need Observables. If we don't have any use-case for it right in that moment (and I couldn't come up with any right now, especially since the setup phase in the new platform is allowed to be async, we can basically wait for whatever we need for registration), we might not want to build it now, but imho we should expect to build it as soon as we hit that use-case.

Code sample

Just for clarification what I mean by the frozen registry:

// NP setup life-cycle
function setup () {
  const visualizationRegistry = createRegistry();

  return {
    visualizationRegistry,
  };
}

// NP start life-cycle
function start () {
  return {
    // Or however we get access to the previously created object
    visualizationRegistry: this.visualizationRegistry.freeze(),
  };
}

With:

freeze: Pick<Registry, 'get' /* and potentially all other getter methods */>;

So the freeze method returns a stripped down instance not even having the modification functions anymore. Besides that I think it should still "lock down" the original registry, in case someone keeping an reference around that and trying to add further things once it's frozen down, so we can throw an error in that case.

[stacey-gammon]

So... I started cleaning up some stuff in my embeddable PR in the registries and I'm essentially just going to use this method locally. When the official version is ready, it should be easy to migrate over.

[epixa]

The idea of leaning on the lifecycle hooks to keep the registry simple is good, but in practice this is going to be a challenge. This registry concept needs to support the legacy world during this migration, and that is going to be around for 18 months. The legacy world doesn't have these lifecycle hooks, so I don't see a way around going the observable route.

Even with lifecycles, observable backed state makes sense in many situations. They don't necessarily add a ton of complexity in practice for consumers, and they help future proof your API as your needs change over time.

[streamich]

List of our registries:

• Embeddables
  • EmbeddableFactoryRegistry — converted to Registry, converted to Map.
  • triggerRegistry — converted to Registry, converted to Map.
  • actionRegistry — converted to Registry, converted to Map.
• Interpreter
  • FunctionsRegistry — moved to NP, possibly will convert to Map.
  • RenderFunctionsRegistry — moved to NP, possibly will convert to Map.
  • TypesRegistry — moved to NP, possibly will convert to Map.
• uiRegistry
  • chromeHeaderNavControlsRegistry — ShieldUser, kbnBaseUrl, Private, $http, chrome, activeSpace
  • chromeNavControlsRegistry — no Angular, possibly not used.
  • VisTypesRegistryProvider
  • VisEditorTypesRegistryProvider
  • VisRequestHandlersRegistryProvider — de-Angularized, possibly not used.
  • VisResponseHandlersRegistryProvider — de-Angularized, possibly not used.
  • ActionDefaultsRegistryProvider — de-Angularized.
  • panel_registry.js — only one entry in registry; Private, config, $rootScope, $compile
  • ContextMenuActionsRegistryProvider
  • EmbeddableFactoriesRegistryProvider — this will be replaced by new Embeddables?
  • IndexPatternCreationConfigRegistry — Angular-free. Converted to Registry, converted to plain array [].
  • IndexPatternListConfigRegistry — Angular-free.
  • DevToolsRegistryProvider — $window, $http. Shim dev tools #49349
  • DocViewsRegistryProvider — @kertal refactored it (De-angularize DocViewer #42116, De-angularize DocViewer table layout #43240)
  • RegistryFieldFormatEditorsProvider — Angular-free, but with constructor.
  • NavBarExtensionsRegistryProvider — seems to be empty. 🙃@lizozom this is done, right?
  • SavedObjectRegistryProvider — savedSheets.
  • ShareContextMenuExtensionsRegistryProvider — Private, i18n, dashboardConfig (@flash1293 is looking into this)
  • FeatureCatalogueRegistryProvider — i18n, logstashLicenseService Migrate ui/registry/feature_catalogue to New Platform plugin #48818
• search_strategy_registry
• index_management_extensions
  • summaryExtensions
  • actionExtensions
  • bannerExtensions
  • filterExtensions
  • toggleExtensions
  • badgeExtensions
• Canvas
  • ElementsRegistry
  • TagRegistry
  • TemplateRegistry
  • TransitionsRegistry
  • DatasourceRegistry
  • transformRegistry
  • modelRegistry
  • ArgTypeRegistry
  • viewRegistry
• InspectorViewRegistry — moved to NP as internal implementation (#42164)
• ExportTypesRegistry
• user_profile_registry.ts
• savedObjectManagementRegistry
• registerSettingsComponent
• FeatureRegistry
• AggTypeFilters
• AggTypeFieldFilters
• EditorConfigProviderRegistry
• FieldFormatRegistry

[streamich]

#37310 — PoC of moving a registry to NP.

[streamich]

Below are totals of number of registered items in each uiRegistry:

Those counts are generated at runtime and change depending on which app you open, but the 27 items in featureCatalogue registry is the highest number I have seen.

Discussed with @spalger, for a list of 27 items we don't need any indexing, so we can remove IndexedArray backing of registries.

It should be super fast to iterate though 27 items, or even 100 items. So, instead of indexing we can add a .find() and .filter() properties to registry.

type Predicate<T> = (item: T) => boolean;

interface Registry<T> {
  // ...
  find: (predicate: Predicate<T>) => T | undefined;
  filter: (predicate: Predicate<T>) => T[];
}

Other options:

• We can memoize .find() and .filter() invocations.
• We can implement some indexing, but simpler than what IndexedArray does.

[streamich]

It looks like some uiRegistrys are not used. Most uiRegistrys have 3 or less entries, maybe those should not be registries at all.

[epixa]

It looks like some uiRegistrys are not used. Most uiRegistrys have 3 or less entries, maybe those should not be registries at all.

I agree completely with this. I think a centralized registry pattern is over engineering for problems that don't actually exist in our code base. Even if it does make sense for something like visTypes (I don't think it does), that doesn't mean it needs to be a shared abstraction.

At the scale we're talking about, sorting arrays and finding elements in an array is fast and easy. And it requires no extra knowledge or migration efforts outside of native JavaScript.

I've been saying this for years, but at some point I recognize I'm shouting into the wind.

[streamich]

We decided to NOT have a common implementation for registries. Instead, registries can internally have any implementation and can be replaced by some semantically named function like addIndexPatternType():

In the NP that function could be exported as plugin contract, but registry itself kept private:

class Plugin {
  indexPatternTypes = [];
  setup () {
	return {
      addIndexPatternType: type => this.indexPatternTypes.push(type),
    };
  }
}

[timroes]

Even if it does make sense for something like visTypes (I don't think it does), that doesn't mean it needs to be a shared abstraction.

I unfortunately don't understand what you're suggesting here @epixa? What would you suggest instead of using a registry? Maybe I am just understanding that sentence wrong, but the second half of the sentence sounds like you doesn't just mean using a shared abstraction doesn't make sense for visTypes, but using of "registries" altogether doesn't make sense?

I've synced with @ppisljar this morning about that and here a couple of points we discussed, why it might make sense still implementing that registries:

• As mentioned above a couple of those registries don't boil down to pure array/map, but need something like sorting (e.g. the InspectorsView registry or the DocViewerRegistry), it might be comfortable having a common implementation for that.
• But way more important: In the most places we want to separate between the different lifecycle events: so that you can only register in the setup phase and only consume in the start phase. It's not that hard to provide that, but it's really likely if there is no common implementation this will just not be handled properly in most "registries". The same is true for exposing the content of the registry in an immutable form. We usually don't want someone that consumes the entries in the registry to mess up the registry itself. In the simple implementation of just using an array or map it's really easy to just return that array in a modifiable form to the consumer. I suspect without a common implementation we'll have this kind of leakage in 80%+ of all registries implemented. That's why I still think we should provide a common implementation that does especially the freezing (separating of lifecycles) and immutability of consumed items easier to use and harder to forget.

[epixa]

I unfortunately don't understand what you're suggesting here @epixa? What would you suggest instead of using a registry?

Having no shared abstraction here and just using native data structures, usually array or map. As far as I understand, that's where the app arch team ended up as well.

Regarding sorting and lifecycle events, I don't share your concerns broadly. I agree that people can introduce bugs when implementing a sort function, and I agree that people can architect their plugins in such a way that dependent plugins can directly mutate the state they manage outside the context of lifecycle, but I think in practice these will be rare.

Don't want someone to add things to your collection outside of the function you provide? Don't give them access to the raw collection, perhaps give them a shallow copy instead.

Don't want someone to add things to your collection after setup? Don't return your registry function in start. And if you're concerned someone will store a reference to the register function and use it later, make it throw an error if your start function has been invoked.

In other words, these aren't just solvable problems, they're problems with trivial solutions.

[streamich]

@streamich I'm curious if we have existing use cases that could take advantage of the observable interface.

@mattkime I just came across Inspector's viewRegistry which is an event emitter, which emits "change" event every time new view is registered.

[lizozom]

@streamich posted this proposed solution for deangularizing situations where you want to get rid of uiRegistry, but keep the registry:

[lizozom]

@streamich @lukeelmers @ppisljar @kertal

Following up on the proposed fix above, I want to make sure we're aligned: We don't want to expose the registries themselves to solution \ 3rd party developers, as before.

The registry itself now becomes an internal implementation detail, and the plugin should expose a registration\add method.

So if previously a developer would import the registry and register to it:

import { DocViewsRegistryProvider } from 'ui/registry/doc_views';
DocViewsRegistryProvider.register(...);

In the new platform, it would look something like:

somePlugin.registerDocView(...);

[streamich]

Re-opening as we use this list for tracking.

[timroes]

Is this issue still up to date? It seems like it has been updated the last time in November. If this is not used anymore, let's close it, please.