Replace client-side context with getStartServices API

[joshdover]

This issue evolved from the discussion in #46483

After seeing this play out in practice in a few different areas, I think we're seeing context on the client-side being used very differently than on the server-side.

Server-side context

On the server-side, we're using context providers to:

1. Simplify consumption of an API. For instance, provide the most recent emitted value of an Observable rather than the Observable itself to a HTTP route.
2. Bind a service to the current request. This allows us to avoid having to pass the request object all over the place just to get user-scoped access to Elasticsearch.

This makes the context providers on the server a bit more complex (and useful) than just the raw APIs they're built on top of. It also means they're more specific to their usages. For instance, a context provider for HTTP routes would be different than a context provider for a background task.

Client-side context

On the client-side, things are a bit different:

1. We're really just injecting services into functions, unchanged from their raw form
2. Exposing "fixed" versions of a service don't quite make sense for many of the rendering contexts because it's quite likely that we'd want the UI to adapt to Observables. This is different than HTTP routes on the backend because rendering contexts are long-lived.
3. It's hard to imagine fundamentally different types of usage patterns like we might see on the server. Almost all client-side code is ultimately concerned with showing something to the user. Even the "search" context mentioned by Stacey above should probably adapt to Observables changing so that UI that depends on these search APIs adapt to an setting change.

Ultimately, many of the motivations in the original RFC don't apply to the use-cases we see on the client-side.

Since we're using context providers in such a simple way on the client, it does feel like a lot of unnecessary complexity just to get access to some core APIs.

Alternative: Service binder

This leads me to believe that maybe what we really need on the client is a simple way to bind Core and Plugin APIs to a function:

interface CoreSetup {
  bindServices<T extends (...args: any[] => any)>(
    callback: (core: CoreStart, plugins: object) => T
  ): T;
}

class Plugin {
  setup(core) {
    core.application.register({
      id: 'myApp',
      mount: core.bindServices(
        (core: CoreStart, plugins: MyPlugins) => ({ element }) => {
          // normal app mounting code here
        }
      ),
    });
  }
}

This bindServices method would still solve one of the core problems that context currently solves on the client: getting access to APIs available only during the start lifecycle for functions registered during setup.

Advantages of this approach:

1. Neither registries nor calling plugins would necessarily need to know about bindServices at all.
2. This eliminates the potential bug highlighted in the context service documentation where a plugin could use the context service incorrectly, resulting in handlers getting the wrong dependencies.
3. Can be used deeply inside a UI or service tree to get access to Core APIs without having to pass all of them down.

Basically this bindServices function would be a very simple dependency injector that would respect the calling plugin's dependencies and does not introduce any magic to get any dependency in the system.

Disadvantages:

1. We lose the ability to expose "fixed" services such as resolving Observables. However, as noted in the difference (3) listed above, this is not something we generally want to do on the client.
2. We cannot tailor APIs to specific contexts. However, I have yet to see a use-case for this on the client.
3. This may limit what we could remove from CoreStart as proposed by a pending RFC though that RFC is primarily focused on the server-side.

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[joshdover]

After experimenting with the implementation here, I'm leaning towards a slightly different API, similar to what @restrry brought up a couple weeks ago:

interface CoreSetup {
  getStartServices(): Promise<[CoreStart, TPluginStartDependencies]>;
}

Benefits:

• Eliminates the situation where bindServices gets called before start APIs are available, resulting in an exception.
• A bit more ergonomic
• Implementation is much simpler in Core

Drawbacks:

• Can only be used in async code
• A little more awkward to mock out in tests

[rudolf]

I like the revised suggestion. Even though the type signature helps, it's not obvious that this is really just returning the CoreStart API's. To keep the naming consistent can we rename the method to getCoreStart? :

interface CoreSetup {
  getCoreStart(): Promise<[CoreStart, TPluginStartDependencies]>;
}

[joshdover]

To keep the naming consistent can we rename the method to getCoreStart?

I think this name isn't quite right either though, since it returns more than just CoreStart. Maybe a more helpful name would be getStartContracts?

[pgayvallet]

It's quite late in the decision process so I won't interfere, however I'm not a big fan of the revised proposal to be honest, as it makes quite unclear on when this will actually be resolved, and feels less like a surface API than the initial one imho.

I think I did prefer the initial proposal. I would probably have gone for something like <T>whenCoreStart(({core, pluginDependencies}) => Promise<T>) ?

would be used like

core.application.register({
      id: 'myApp',
      title: 'My App',
      async mount(context /* deprecated */, params) {
        return setup.whenCoreStart(({core, dependencies}) => { // or `setup.afterStartup` or something
           const { renderApp } = await import('./application');
           return renderApp(core, dependencies, params);
       });        
      }
    });

instead of the current implementation of #50231 which is

core.application.register({
      id: 'myApp',
      title: 'My App',
      async mount(context /* deprecated */, params) {
        const [coreStart, depsStart] = await core.getStartServices();
        const { renderApp } = await import('./application');
        return renderApp(coreStart, depsStart, params);
      }
    });

I know it's closer to the original core.bindServices proposal, but I think I find it a little more clear. Else I think I would at least makes the accessors more explicit by something like getStartContractsWhenStarted. But we can also decide that proper documentation is enough on that one.

Because when I see the upsides of the alternative,

Eliminates the situation where bindServices gets called before start APIs are available, resulting in an exception.

I'm not sure to understand the actual issue? bindServices receive a handler, we choose when to execute the handler, and we do after core start, so how calling it before start APIs are available is an issue?

A bit more ergonomic

I personally find the wrapper of handler way more elegant that a raw promise accessor.

Implementation is much simpler in Core

I don't realise at which point this is simpler, so I won't argue with this.

[joshdover]

Just saw this today. Will write up my thoughts when I get a bit more time this week.

[joshdover]

I'm not sure to understand the actual issue? bindServices receive a handler, we choose when to execute the handler, and we do after core start, so how calling it before start APIs are available is an issue?

Right, with your proposal above, this would not be a problem. The problem with the initial proposal had to do with how it returned a sync function that couldn't be called until start dependencies had been resolved or it had to be made async.

Given those two options, it didn't seem to me that this factory function pattern was very beneficial since it couldn't be used transparently.

I prefer the raw Promise accessor pattern personally since it's a bit simpler to mock out.