[Discuss] Generics, registries, type mappings, factory overrides

[stacey-gammon]

What

There are two usage patterns I have been thinking about lately, that are somewhat intertwined:

1. Discouraging typescript generics with registries
2. Encouraging a pattern for plugins adding enhancements to an instance/implementation supplied from another plugin.

Type generics on registries

There are two ways to access registry items:

  // The id value is not known at compile time.
  const foo = fooRegistry.get(id);

  // The id value is known at compile time.
  const fooBar = fooRegistry.get<FooBar>(FOO_BAR_ID);

Using a generic type in the former case does not make sense and will only be a source of bugs, since you don't actually know the value of id. The second case is a pattern we use now, but there is a better way:

  const fooBar = fooBarPlugin.getFooBar();

Getting the instance, correctly typed, from the registering plugin directly is safer because it will require the user to list fooBarPlugin as a direct dependency.

This is all well and good... until I started thinking about the pattern for the second situation.

Plugins adding enhancements

A pattern to support this is to provide a setCustomProvider function in the setup method of the plugin that is providing the base implementation.

// fooPlugin.ts
export class FooPlugin {
  setup() {
    return {
      setCustomFooProvider(customFooProvider: (def: FooDefinition) => Foo) {...}
      registerFooDefinition(def: FooDefinition) {...}
    }
  }

  start() {
    return {
      getFoo(id): Foo {...}
    }
  }
}

If we use the above pattern of registering definitions in the setup lifecycle and returning instances in the start lifecycle, we can support an injection point for enhancements. Now however, generics come back into play because the plugin registering a custom implementation didn't create the instance.

// fooBarPlugin.ts
export class FooBarPlugin {
  private fooBarDefinition: FooBarDefinition = new FooBarDefinition();

  setup(core, { fooPlugin }) {
    fooPlugin.registerFooDefinition(fooBarDefinition);
  }

  start(core, { fooPlugin }) {
    return {
      // We can't do this option anymore because this plugin didn't create the fooBar instance,
      // it only has a reference to `fooBarDefinition`. 
      // XX getFooBar(): FooBar => { return this.fooBar } XX

      // Now we need to cast, or use generics on `get`.  Casting is probably the less
      // worse option.
      getFooBar(): FooBar => { return fooPlugin.get(FOO_BAR) as FooBar }
    }
  }
}

The only way to solve this the "best" way would be to introduce more lifecycle methods, so the flow would be

1. Enhanced plugin registers enhancement injection in Phase 1.
2. FooBarPlugin creates an instance of FooBar from FooBarDefinition in Phase 2, and registers it, in Phase 2
3. FooBarPlugin returns a getter to the fooBar instance in Phase 3.

Summary

We have discussed this in the Kibana Repo Review and the best interim path forward is probably to:

• stop supporting generics on registry items
• get rid of type mappings pattern
• use casting
• promote the best practice of compile time access from the registering plugin, not the generic plugin.

The best long term path forward is to figure out how to solve the problem of needing n+1 lifecycles.

[mattkime]

I'm having trouble determining which parts of the example are axioms. Obviously the plugin structure is axiomatic. Is the enhancement registration pattern axiomatic as well?

My summary of the problem -

1. Item A is registered in the setup lifecycle
2. Item B is derived from Item A
3. I need to access to Item B in the start lifecycle.

Can the registration function return the secondary item?

I think I'm confused about the case where we need to use a generic .get function and know the type instead of using a getter off the plugin contract.

I largely agree with what you're saying but would like to rule out the possibility that we're discussing the consequences of other alterable coding decisions.

[stacey-gammon]

Can the registration function return the secondary item?

No, because we don't know if another plugin has yet to set a custom enhancement provider.

[mattkime]

No, because we don't know if another plugin has yet to set a custom enhancement provider.

Can you sketch that out in code? I'm not sure what a custom enhancement provider might look like

[stacey-gammon]

Sure thing, I added a few example plugins in this PR that showcase the pattern:

#62998

This example does not make use of generics, didn't want to over complicate the basic example.

[elasticmachine]

Pinging @elastic/kibana-app-arch (Team:AppArch)

[lukeelmers]

get rid of type mappings pattern

To provide more context around this item, what is being referred to here is the usage of declare module to augment an interface which serves as a global mapping of items in a registry, e.g.

// plugin `foo` exposing a registry
export interface RegistryMapping {}
export registry = {
  get<T extends string>(id: T) => RegistryMapping[T];
}

// plugin `bar` registering an item to a registry
interface BarItem {
  id: string;
  hiya: boolean;
}

declare module '../../../plugins/foo/public' {
  interface RegistryMapping {
    barItem: BarItem;
  }
}

const item: BarItem = { id: 'barItem', hiya: true };
foo.register(item);

// plugin `baz` which consumes `foo` registry and needs access to `barItem`
const i = foo.get('barItem'); // barItem is correctly typed

The suggested alternative would be to prefer to have baz make an explicit dependency on bar, either by building its own mapping similar to the example here, or casting to the desired interface.

[ppisljar]

I am not fully following, but if i understand correclty:
this is not a problem, we don't know the id at compile time, the type we get is a generic type as we need to assume any of the registered items could be returned

  // The id value is not known at compile time.
  const foo = fooRegistry.get(id);

this is a problem:

  // The id value is known at compile time.
  const fooBar = fooRegistry.get<FooBar>(FOO_BAR_ID);

i agree, from two perspectives:

• you are duplicating information, why do you need to pass FooBar type in ? this allows you to pass wrong type for the id you are giving. I think we should just give it ID and the correct type should be returned based on that ID. we already have something like this implemented for agg types and expression functions.

  // The id value is known at compile time.
  const fooBar = fooRegistry.get(FOO_BAR_ID);

there is yet another problem however as someone might just pass in a string:
const fooBar = fooReguistry.get('foo_bar');
this is also a problem, as this plugin now doesn't need to list foo bar plugin as dependency which might lead to weird issues.

to resolve this issue we could say what i think Stacey is suggesting, to actually export getFooBar on the contract, and that the actual registry should only be used when id is not known at compile time (for example dashboard doesn't know what embeddables are there at compile time as new ones can be added thru plugins)

i think this makes sense in some scenarios, but doesn't in others. for example with expressions each plugin might register functions and i think it really doesn't make sense that it would expose a getter on the contract. To get the function you need to get it thru expression plugin which will inject the handlers in it for you.

[stacey-gammon]

You could do it with expressions like this:

type FooExpressionFn = ExpressionFn<...>;

class FooExpressionPlugin {
  private getFooExpressionFn: () => FooExpressionFn;
  
  setup(core, expressions) {
    this.getFooExpressionFn = expressions.registerFn(getMyFooFunctionDefinition()) as FooExpressionFn;
  }

  start: () => ({
    fooExpressionFn: this.getFooExpressionFn()
  })
}

Benefits:

• expressions plugin still has a the opportunity to inject handlers
• no typescript generics on expressions.getFn(id)
• Any user who wants to use the full type version should use fooExpressionStart.fooExpressionFn instead of casting via expression.getFn('foo') as FooExpressionFn

Downsides:

• Still using casting to as but at least the author of FooExpressionFn is doing the casting, not the user of FooExpressionFn.

[ppisljar]

closing this due to inactivity, please re-open if its still relevant