Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add scoped services and plugin contracts #40822

Closed
wants to merge 2 commits into from

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented Jul 10, 2019

Summary

Related to #40554 and #18843

In order to provide pre-configured or scoped dependencies to plugins, we need a pattern for core services as well as a mechanism for Plugin contracts that gives the core a way to support what I'm calling "plugin-scoped" contracts.

"Plugin-scoped" contracts are instantiated by a factory function that takes a plugin identifier and returns the contract for use with that plugin.

This is a primary need in order for plugins to be able to offer handler contexts. Because handler contexts expose functionality from other plugins, they need to be smart enough to handle plugin dependencies so that a handler in PluginA cannot see context provided PluginB unless it explicitly depends on PluginB.

Having knowledge of which plugin is registering a context and which plugin is consuming it allows this to work. Thus, plugin-scoped contacts.

Plugin contracts

In order to support this, this PR adds the ability for plugins to return a function from their lifecycle methods. For example:

/** @public */
export interface MyPluginSetup {
  myApi(): any;
  registerHandler(handler: Handler): any;
  registerContext(name: string, provider: ContextProvider<...>): void;
}

class MyPlugin<MyPluginSetup> {
  setup(core) {
    this.contextContainer = core.context.createContainer();

    return (pluginId: symbol) => ({
      myApi() { ... },
      registerHandler(handler: Handler) {
        // use pluginId here when storing the handler
        // we can use this information later to build a context for this handler
      },
      registerContext: (name, provider) =>
        // Core's ContextContainer uses this pluginId to wire up handlers
        // with the contexts available based on their plugin dependencies
        this.contextContainer(name, provider, pluginId)
    });
  }
}

The passed argument is a symbol rather than the actual plugin ID string to avoid an upstream plugin from coupling any specific behavior to its dependencies based on which plugin is consuming it.

The symbol for a given plugin will be constant throughout the system lifecycle. The plugin system will call this factory function with the ID of the dependent plugin before passing the contract to the dependent plugin.

Core Services

There is no "magic" here that automatically allows a core service to be pre-configured for a plugin. This PR just adds a new pattern and delineates the difference between internal contracts from exposed contracts. Example of the new pattern:

/** @internal */
export interface InternalMyServiceSetup {
  internalOnlyApi(): JSX.Element;
  forPlugin(): MyServiceSetup;
}

/** @public */
export interface MyServiceSetup {
  publicApi$(): Observable<...>;
}

/** @internal */
export class MyService extends CoreService<MyServiceSetup> {
  setup() {
    return {
      internalApi() { ... },
      forPlugin: (pluginId: name) => ({
        publicApi$() { ... }
      }),
    };
  }
}

This pattern also has a couple other side benefits:

  • It moves the responsibility of creating the plugin-scoped version of a core service from the PluginsService (createPluginSetupContext) to the service itself.
  • It eliminates the need to use Pick<ChromeStart, 'blah'> when declaring the public version of the service contract.

Dev Docs

New Platform Plugins may now return a factory function for creating an instance of their public contract from lifecycle methods. This allows plugins to offer scoped versions of their APIs to dependencies.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joshdover joshdover added Feature:New Platform v7.4.0 Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Jul 11, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@joshdover joshdover marked this pull request as ready for review July 11, 2019 20:21
@joshdover joshdover requested review from a team as code owners July 11, 2019 20:21
@joshdover joshdover added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Jul 11, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@spalger
Copy link
Contributor

spalger commented Jul 11, 2019

Do you think it might make sense to switch out pluginId: symbol with something like plugin: readonly { id: symbol }/Object.freeze({ id }) so that it can be used in things like WeakMap and could potentially carry additional metadata in the future?

@joshdover joshdover mentioned this pull request Jul 12, 2019
5 tasks
@joshdover
Copy link
Contributor Author

Do you think it might make sense to switch out pluginId: symbol with something like plugin: readonly { id: symbol }/Object.freeze({ id }) so that it can be used in things like WeakMap and could potentially carry additional metadata in the future?

Exposing only a raw symbol doesn't limit consumers from using WeakMap (consumers could construct an object themselves), but I do like using an object to make this more easily extensible in the future 👍

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

*
* @public
*/
export type PluginLifecycleContract<T> = T extends (dependency: Readonly<{ id: symbol }>) => infer U
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is something that bothers me about the proposed design.

  • Implementation details of the core leak into user code. We can provide a function bound to the pluginId and avoid introducing this concept directly within the plugin code.
function createPluginSetupContext(coreContext, deps, plugin) {
  // ....
  registerContext : (contextName, provider) =>
    contextContainer.register(contextName, provider, plugin.name)
}
  • IMO now it's harder to reason about the sequence of all steps. The previous design guaranteed us that value, returned from a lifecycle method, was created when a lifecycle method was called.

Copy link
Contributor Author

@joshdover joshdover Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, I'm not in love with this pattern myself, but I don't think what you've proposed works.

  • A plugin can expose it's own context container for other plugins to register context providers on.
  • Whenever a new context is registered by another plugin, we need to be able to keep track of which plugin registered that context.
  • Whenever a handler is invoked that belongs to a plugin, we also need to keep track of which plugin registered that handler.
  • Given this information, the context container can create a context for the handler that only include contexts registered by plugins that the handler's plugin depends on. (wow that's a mouthful).

I don't see a great way of providing this information besides a factory function.

We could do some kind of implicit binding. For instance, we could automatically bind any contract functions called registerContext with the dependent's pluginId:

class Plugin {
  start() {
    return {
      registerContext: contextContainer.register
    }
  }
}
// When wiring up dependencies in Core's PluginsService

const pluginContract = Object.keys(contract).reduce((acc, key) => {
  if (key === 'registerContext') {
    acc[key] = (contextName, provider) =>
      contract.registerContext(contextName, provider, pluginName);
  } else {
    acc[key] = contract[key];
  }
  return acc;
}, {});

// ...

I just don't like this auto-magic compared to a fairly straightforward factory function that can be expressed in the type system. Any other ideas?

@@ -126,13 +132,15 @@ export class ElasticsearchService implements CoreService<ElasticsearchServiceSet
return {
legacy: { config$: clients$.pipe(map(clients => clients.config)) },

adminClient$: clients$.pipe(map(clients => clients.adminClient)),
dataClient$: clients$.pipe(map(clients => clients.dataClient)),
forPlugin: () => ({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why service should know who and how uses API?
what if other core service wants to use any elasticsearch service api? Say, we re-write SavedObject and now it wants to use elasticseach service. Will it call forPlugin?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Services can still expose APIs to other services through their Internal contract which may be similar to the plugin-specific one.

The reason I prefer this is it moves the responsibility of constructing the exposed "public" contract to each service rather than having to constantly update the PluginsService to produce this value.

@joshdover joshdover mentioned this pull request Jul 15, 2019
4 tasks
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joshdover
Copy link
Contributor Author

Closing this PR while I experiment with an alternative idea.

@joshdover joshdover closed this Jul 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants