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 ContextService #41251

Merged
merged 5 commits into from
Jul 30, 2019
Merged

Add ContextService #41251

merged 5 commits into from
Jul 30, 2019

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented Jul 16, 2019

Summary

This adds a generic implementation for leveraging the Handler Context pattern.

This is a slightly updated version of the service proposed by #40554

Basic Example

interface MountContext {}
type MountArgs = [HTMLElement];
type MountReturn = () => void;

interface App {
  mount: (context: MountContext, ...mountArgs: MountArgs) => MountReturn
}

Setting up a ContextContainer and configuring handlers

export class ApplicationService {
  private readonly apps$ = new BehaviorSubject<App[]>([]);

  public setup({ context }) {
    this.mountContext = context.createContextContainer<
      MountContext,
      MountArgs,
      MountReturn
    >();
    return {
      registerApp: (plugin: symbol, app: App) =>
        this.apps$.next([
          ...this.apps$.value,
         {
           ...app,
           mount: this.mountContext.createHandler(plugin, app.mount)
         }
      ]),
      registerContext: this.mountContext.registerContext
    };
  }

  public async start() { ... }
}

Invoking a configured handler

export class ApplicationService {
  private readonly apps$ = new BehaviorSubject<App[]>([]);

  public setup() { ... }

  public async start({ context }) {
    const mountContext = context.createContextContainer<ApplicationContext>();
    
    // dumb example of mounting an app
    const appToMount = this.apps$.value[0];
    // `appToMount.mount` will automatically invoke each context provider and
    // call the original mount function with it
    const unmount = appToMount.mount(document.createElement('div'));

    return {
      registerContext: mountContext.registerContext
    };
  }
}

Details

This approach does leak details about consuming plugins to context service owners via the plugin argument that must be passed. We evaluated a few approaches and have decided that this had the least-bad consequences. If we decide that this level of detail is dangerous, we could go a step further and use some sort of opaque identifier such as a symbol generated by Core for each plugin.

To do in future PRs:

  • Add a parallel service to the server-side

Dev Docs

New Platform plugins may now implement their own context API on the server or client.

A IContextContainer can be used by any Core service or plugin (known as the "service owner") which wishes to expose APIs in a handler function. The container object will manage registering context providers and configuring a handler with all of the contexts that should be exposed to the handler's plugin. This is dependent on the dependencies that the handler's plugin declares.

class MyPlugin {
  private readonly handlers = new Map();
  setup(core) {
    this.contextContainer = core.context.createContextContainer();
    return {
      registerContext(pluginOpaqueId, contextName, provider) {
        this.contextContainer.registerContext(pluginOpaqueId, contextName, provider);
      },
      registerRoute(pluginOpaqueId, path, handler) {
        this.handlers.set(
          path,
          this.contextContainer.createHandler(pluginOpaqueId, handler)
        );
      }
    }
  }
}

Checklist

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

For maintainers

@elasticmachine

This comment has been minimized.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joshdover joshdover added 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 labels Jul 16, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@joshdover joshdover marked this pull request as ready for review July 16, 2019 17:46
@joshdover joshdover requested a review from a team as a code owner July 16, 2019 17:46
@elasticmachine
Copy link
Contributor

💔 Build Failed

@joshdover
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@epixa epixa mentioned this pull request Jul 16, 2019
10 tasks
src/core/public/context/context_service.ts Outdated Show resolved Hide resolved
src/core/public/context/context_service.ts Outdated Show resolved Hide resolved
src/core/public/context/context.ts Outdated Show resolved Hide resolved
src/core/public/context/context.ts Outdated Show resolved Hide resolved
@joshdover
Copy link
Contributor Author

Still working on some of the updates requested here but have to go for the day, should have this updated tomorrow.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joshdover joshdover force-pushed the np/context-service-2 branch 2 times, most recently from 068db26 to 7bb0177 Compare July 23, 2019 18:36
@elasticmachine
Copy link
Contributor

💔 Build Failed

@joshdover
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mshustov
Copy link
Contributor

ack: will review by the end of the week

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

src/core/public/context/context.ts Outdated Show resolved Hide resolved
src/core/public/context/context.ts Outdated Show resolved Hide resolved
src/core/public/context/context.ts Outdated Show resolved Hide resolved
src/core/public/context/context_service.test.ts Outdated Show resolved Hide resolved
src/core/public/plugins/plugins_service.test.ts Outdated Show resolved Hide resolved
src/core/public/context/context.ts Outdated Show resolved Hide resolved
src/core/public/context/context.ts Outdated Show resolved Hide resolved
i18n: I18nStart;
uiSettings: UISettingsClientContract;
}
[contextName: string]: unknown;
Copy link
Contributor

Choose a reason for hiding this comment

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

is it a necessary declaration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not. I thought it could be useful to help show that the contextName parameter in registerContext corresponds to keys on this object.

src/core/public/context/context_service.ts Outdated Show resolved Hide resolved
src/core/public/context/context.ts Outdated Show resolved Hide resolved
this.contextNamesBySource = new Map<ContextSource, Array<keyof TContext>>([[coreId, []]]);
}

public registerContext = <TContextName extends keyof TContext>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate on why we are using arrow functions here instead of class methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without using function properties, I was having issues with this being the caller. I'll play around with this more to see if it's necessary.

src/core/public/index.ts Outdated Show resolved Hide resolved
@@ -17,7 +17,7 @@
* under the License.
*/

export function mapToObject<V = unknown>(map: Map<string, V>) {
export function mapToObject<V = unknown>(map: ReadonlyMap<string, V>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this have any consequence in trying to pass in a Map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function doesn't modify the argument and Map is compatible with ReadonlyMap so this change just makes this function more generally useful.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@mshustov mshustov left a comment

Choose a reason for hiding this comment

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

Good start. Let's try API with real use cases for the application and HTTP services.

@joshdover joshdover merged commit 76b0fbe into elastic:master Jul 30, 2019
kibana-core [DEPRECATED] automation moved this from Code review to Needs Backport Jul 30, 2019
@joshdover joshdover deleted the np/context-service-2 branch July 30, 2019 18:34
joshdover added a commit that referenced this pull request Jul 30, 2019
@joshdover joshdover moved this from Needs Backport to Done (this week) in kibana-core [DEPRECATED] Aug 1, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

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.

None yet

4 participants