[rfc] "setup" lifecycle handler for core and plugins #32507
Conversation
|
Pinging @elastic/kibana-platform |
The `init` lifecycle function for core and plugins will be for one-time setup and configuration logic that should be completed in a finite amount of time rather than be available throughout the runtime of the service. The existing `start` lifecycle function will continue to serve only the purpose of longer running code that intentionally only executes when `init` is finished.
There was a problem hiding this comment.
Would init functionally replace a constructor / factory function? I'm having a hard time thinking of a situation where we'd need both. If so, the export interface could simply be:
export const plugin = {
init(core, plugins) {},
start(core, plugins) {},
stop(core, plugins) {},
}If the plugin still wants to use a class, they certainly can, but no arguments would be provided to the constructor because the instance would be need to be constructed on import.
Personally, I like this approach as it makes the implicit lifecycle event of construction go away and initialization is now more in line with other lifecycle events.
rfcs/text/0001_lifecycle_init.md
Outdated
|
|
||
| Adoption will need to be manual. Since the bulk of the `start` logic in the | ||
| repo today is configuration-oriented, I recommend renaming `start`->`init` in | ||
| all services and plugins, and then adding an empty `start` where it is |
There was a problem hiding this comment.
What do you think about making each lifecycle method optional? That way, plugins that don't need separate init and starts don't have to have that complexity in their exported plugin. Establishing this pattern now would also make adoption easier for future lifecycle additions.
There was a problem hiding this comment.
I think that is a reasonable discussion to have, but I think that's a separate discussion from this proposal.
It would not replace the constructor.
Technically speaking, the plugin class is a convention. The plugin service consumes a factory function exposed by the plugin module, and by convention that factory function just instantiates its Plugin class and passes the argument(s) from the factory to the class constructor. So plugins could theoretically describe themselves as in your example. That said, I don't think plugins in the Kibana repo should vary how they define themselves, and I don't think we should encourage others to do so regardless of what is technically possible. Most plugins are going to be stateful, and classes are a good way to represent stateful things. |
This must be where my confusion lies. In discussions I've had with @spalger, I've come to understand that while service lifecycle methods can be async, plugin lifecycle methods are not and if a plugin needs to expose some async functionality that it should use an Observable or Promise as it's return value that dependents will need to subscribe or await on their own. The PluginInitializer return type affirms this as well. Are you stating that:
In either case, the constructors and the init lifecycle event have to be executed in some order. If you could put some code in the constructor, it would also work fine in the init. The only case I can think of that would violate this is if we had a core service that was available in the constructor but not in the init. That seems unlikely. In the spirit of reducing the number of decisions a developer has to make, it seems like having one place to do "init stuff" makes sense and would force plugins to follow a consistent pattern. Eliminating the constructor would also make the integration points between the plugin and Core more homogenous. One argument I can see for preserving a constructor would be to give Core more control over when code is executed. It may be that we don't even want plugins to start doing anything in some cases, and if they constructed on import, this would be more difficult to manage. That said, there's nothing really stopping a plugin from doing that with either pattern, just that construction-on-import would be the likely default in a constructor-less pattern. |
|
Just noticed that I'm mistaken and the plugin loader does await the return value. The PluginInitializer type makes sense to me given that. Disregard that part of the reply. |
|
Just confirming so everyone is on the same page: every lifecycle function (init, start, stop) for core services and plugins should always be executed asynchronously. For core services, we'll await the function before executing the next service's equivalent. For plugins, we'll await a lifecycle function before executing the next appropriate lifecycle function in the dependency graph. In all cases, the value returned from the async function (the value that fulfills the promise) becomes the lifecycle contract. Regardless of whether we introduce
A class's constructor should be used to set up state for that class. So, for example, you'd use the constructor to instantiate an empty Set to use as a registry of extensions so that it was available to any and all of its lifecycle functions. Then returned from init you might provide an ability to register extensions on that internal Set, and in start you might invoke all of the extensions, and in stop you might clear the Set so the extensions have the option of being GC'd. Roughly: class Plugin {
constructor() {
this.extensions = new Set();
}
init() {
return {
registerExtension(extension) {
this.extensions.add(extension);
}
};
}
start() {
this.extensions.forEach(extension => extension());
}
stop() {
this.extensions.clear();
}
}It is true that the plugin service will invoke the lifecycle functions in order, but we shouldn't be initializing state in a way where calling functions in a different order might result in exceptions due to accessing undefined variables and such. This is why the concept of a constructor exists - you can rely on it having run before any method is called, so you don't need to write a bunch of defense checks and whatnot every time you want to access state. I certainly should be able to unit test Perhaps another way to look at these things is this:
You could represent this same sort of separation of concerns with a factory: function factory() {
// equivalent of constructor
return {
init() {}
start() {}
stop() {}
};
}And since the interface to the plugin service is a factory function, it doesn't strictly matter to core which approach a plugin takes. But since it doesn't matter from a technical perspective, it mostly just comes down to a matter of opinion. We chose to go with classes because they are a near-universally understand way to express stateful objects.
This paragraph lost me. What does |
Because in legacy platform exists only
My question: if init in NP meant to be used for pre-start configuration, does it make sense to name it configure/setup/whatever ?
This also confused me when reading NP codebase. Should we cover this part in the NP overview docs? And cover some recommendation as well, because now we can do both // first option
async start(){
const foo = await bar();
return {
getFoo: () => foo
}
}
// second option
start(){
return {
foo$: Observable.fromPromise(...)
}
} |
I'm open to this. My preference would be
It seems like we should, yeah. In the end, there are few situations where you should block on asynchronous stuff in your lifecycle functions but in order to support those few times where it makes sense, we have to assume all lifecycle functions are async. |
|
Here's a concrete example of what I'm talking about: What is being proposed// my_plugin/server/index.js
class MyPlugin {
constructor(core, myVal) {
// should I do stuff with core here?
}
init(core, plugins) {
// or do I do it here?
}
}
// Core calls the factory function with some subset of core services
export const plugin = (core) => new MyPlugin(core, 2);No core-specific constructorThis option is less ambiguous in where core-specific setup should happen. // my_plugin/server/index.js
class MyPlugin {
constructor(myVal) {
// no core now, now it's clear I only setup internal state
}
init(core, plugins) {
// do core initialization here
}
}
// Core calls the factory function without any core services to get an instance
export const plugin = () => new MyPlugin(2);construction-on-importAfter thinking about this option more I think this is a bad solution since it forces the plugin to be singleton and could limit testing. // my_plugin/server/index.js
class MyPlugin {
constructor(myVal) {
// no core now, now it's clear I only setup internal state
}
init(core, plugins) {
// do core initialization here
}
}
// No factory function, the module exports an initialized singleton
export const plugin = new MyPlugin(2); |
|
Thanks for clarifying. I agree on your last point - avoiding global singletons is a defined goal of the new platform, and I'm not willing to make an exception for this. Regarding the other two, it's worth noting that the arguments to On the server, we give constructors access to the logger, the config (from kibana.yml), and the environment (os, fs location, etc). We intentionally kept this set of functionality as minimal as we realistically could, but we think it's reasonable to use those three things when setting up your local state or at any time throughout the plugin's lifecycle. |
|
Great, thanks for the context and sorry if the discussion went a bit off the rails. |
|
@joshdover Thanks for working through those details with me. Clearly our overall lifecycle system deserves some better documentation! I pushed a change that renamed this new lifecycle function from |
Co-Authored-By: epixa <court@epixa.com>
|
@restrry Good call, done. |
|
With no open questions and two approvals on the RFC, it seems this is a viable candidate for inclusion in Kibana. Consider this the start of the final comment period. If there are no addressable comments or objections in the next 3 working days, we'll accept the RFC and merge no later than Tuesday, 12 March 2019. |
I may be jumping the gun here, but just to confirm: assuming this RFC is accepted, will the guidance for shimming legacy plugins be that we should use It's a trivial change, so I'm not too concerned about it, but would like to adopt the recommended convention as we begin the migration process. |
|
@lukeelmers Yep, you got it. |
|
With no new comments, this RFC is accepted. Thanks for participating, y'all! |
The
setuplifecycle function for core and plugins will be forone-time initialization and configuration logic that should be completed
in a finite amount of time rather than be available throughout
the runtime of the service.
The existing
startlifecycle function will continue to serveonly the purpose of longer running code that intentionally only
executes when
setupis finished.[skip-ci]