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

Enhancements pattern #62998

Conversation

stacey-gammon
Copy link
Contributor

@stacey-gammon stacey-gammon commented Apr 8, 2020

This PR adds a few example plugins to showcase the enhancements pattern discussed in #62815

From the Enhancements_pattern_plugin readme:

Enhancements pattern

This plugin shows a recommended pattern for how one plugin can enhance another plugins functionality when dealing with a registry of items.

There are three key pieces. The plugin that creates the registry should:

  • Expose a setCustomProvider function in the Setup contract.
  • Expose the ability for other plugins to register definitions in the setup contract.
  • Expose the ability for other plugins to retrieve instances in the start contract.

There are three plugin associated with this example.

  • the greeting plugin exposes a registry of greetings. The default provider uses the very basic alert function to greet the user.
  • the greetingEnhanced plugin registers a custom greeting provider which uses an EuiModal to greet the user with improved stylign.
  • this plugin, enhancementsPatternExplorer registers a few example greetings as well as an app to expose the greet functionality.

To see how this works, first run Kibana with nothing in your kibana.yml via yarn start --run-examples. Navigate to the Enhancements pattern app and see how the greetings look.

Then, stop kibana and edit kibana.yml to turn the greetingEnhanced plugin off by adding this line:

greeting_enhanced.enabled: false

Restart kibana and go through the same motions, and you should now see just the basic alert window.

Screenshots:

Screen Shot 2020-04-08 at 1 50 08 PM

Screen Shot 2020-04-08 at 1 54 13 PM


Once I get buy in on this pattern I will add tests for the example plugin.

closes #62815

@mattkime
Copy link
Contributor

Here's a PR that avoids using ids - stacey-gammon#14

@stacey-gammon
Copy link
Contributor Author

Thanks @mattkime I like that approach! I merged your changes then made some additional changes on top. The greeter implementations have been moved to their own registry to highlight when enhancementsPlugin should access greeters off the registry or off the registering plugin directly.

@stacey-gammon stacey-gammon force-pushed the 2020-04-08-enhancements-pattern branch 2 times, most recently from 41f1cc9 to d2cd642 Compare April 10, 2020 15:05
(this.greetingProvider = customProvider),
registerGreetingDefinition: (greetingDefinition: GreetingDefinition) => {
this.greetingDefinitions[greetingDefinition.id] = greetingDefinition;
return () => this.greetingProvider(greetingDefinition);
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't returning () => this.greetingProvider(greetingDefinition) from within the setup lifecycle potentially cause issues if this is called before another plugin has the chance to call setCustomProvider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, good catch.

how about this change? (sorry I shouldn't have squashed the second commit, force of habit).

it ensures that an error is thrown if the accessor is used before this plugin's start lifecycle.

@stacey-gammon stacey-gammon force-pushed the 2020-04-08-enhancements-pattern branch from d2cd642 to f25c73f Compare April 10, 2020 16:53
examples/greeting/public/plugin.ts Outdated Show resolved Hide resolved
this.greetingDefinitions[greetingDefinition.id] = greetingDefinition;
return () => {
if (!this.greetingProvider) {
throw new Error('Greeters can only be retrieved after setup lifecycle.');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think throwing an explicit error here is definitely a step in the right direction. There's still the potential for a run-time exception. However, this already exists with the way that the NP setup/start lifecycles work in practicality after getStartServices was introduced...

@kobelb
Copy link
Contributor

kobelb commented Apr 10, 2020

I've been trying to think of a different name for this besides "enhancements pattern", but naming is hard and I haven't come up with anything. Conceptually, it seems like we're combining the registry pattern and a variant of the factory pattern where the factory itself can be replaced...

@stacey-gammon
Copy link
Contributor Author

Yea. Maybe this can all be encapsulated under "best practices when creating and using registries" and that includes:

  • How to support the use case when you want to allow another plugin to add enhancements to the base implementation of your registered items.
  • Accessing specific implementations off the registering plugin directly instead of off the generic plugin that exposed the registry.

Once #59189 is done I would group all these multiple new plugins under a single folder. The examples folder right now is difficult to grok quickly because so many examples are split between multiple plugins.

@mattkime
Copy link
Contributor

I've been trying to think of a different name for this besides "enhancements pattern", but naming is hard and I haven't come up with anything.

Registry with enhanced items? Seems like 'registry' should be in there somewhere.

@stacey-gammon stacey-gammon force-pushed the 2020-04-08-enhancements-pattern branch from 05b57fc to 42eba3f Compare April 29, 2020 21:20
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💔 Build #44201 failed 05b57fc16b8602d3a72ebddd85842c3a75c9b1eb
  • 💚 Build #40207 succeeded dc647febb7d94b4d6339617c5ab72fc7f2b959e3
  • 💚 Build #40197 succeeded f25c73f397470994e6aceb7b73eb569e439aef4e
  • 💚 Build #40158 succeeded d2cd64211d67d03e1e870e0586d4b923dc3339b7
  • 💔 Build #40130 failed 6b55c41695a2de512ded3c3bc3b36538da2fd094

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@ppisljar
Copy link
Member

ppisljar commented May 5, 2020

sorry to provide feedback late.

I am not sure how this relates to registries ? it seems that the fact that there is a registry and that we are registring item definitions in it does not affect this pattern, and we could use pattern like this to extend any functionality of any service where that would be requeired. Due to the fact that this can only be done from one place i think this should be limited to our internal usage (x-pack plugins extending the oss) and as such hidden behind a __internal__ namespace. So for this example i think we could drop the registry (to make it less complicated), have the greeting always just display a single greeting and then use the setCustomProvider function to change the way that greeting is displayed.

The registry with item definitions would allow us to do another thing that is not showcased in this examples (unless i missed it), that is extending the actual registered items. Lets say pugin A exposes a registry, plugin B adds few items to this registry, plugin C overrides the item in the registry with a new item, that extends the original item.

i guess we could collapse all into single example, but imo this way both of the things we want to show will be less clear so i would vote for having two separate examples, and the first one not using registry to make it clear this kind of pattern can be used to extend any functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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