Plugin system should fail if Setup lifecycle functionality is called after the setup lifecycle completes.

[stacey-gammon]

Using setup functionality after the setup lifecycle completes is a common anti-pattern I've seen.

Consider the following code:

class SumPlugin extends Plugin {
  this.sum = 0;
  setup: () => ({ add: (i: number) => this.sum += i });
  start: () => ({ getSum: => this.sum });
}

Plugins should be able to assume that by the time the start lifecycle runs, no functionality on setup will be used. In other words, the return value of start.getSum from above is stable and will not change.

Unfortunately, it's very easy for devs to do something like this:

class MyPlugin extends Plugin {
  setup(core, { sum }) {
    this.sum = sum;
  }
  start() {
    this.sum.add(5);
  }
}

This raises two questions:

1. Should accessing setup functionality after the setup lifecycle completes be considered an anti-pattern?
2. If so, how can we ensure devs don't do this accidentally?

If we agree on 1. we can add docs for 2, but I don't think docs are sufficient. I think we should see if the plugin system itself can detect this and fail, by wrapping plugin lifecycle methods and failing if they are called after the setup lifecycle completes.

cc @lukeelmers @kobelb @clintandrewhall

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[clintandrewhall]

I'd love further clarification on what constitutes setup vs start and mount. I think a lot of us gloss over the differences to "get things working"-- a remnant of the conversion to NP. Canvas desperately needs an overhaul in this respect, but it's a tech debt task that's been put off because "it's working" and there's no clear benefit to untangling the mess, (aside from the satisfaction, of course).

I also think we need to consider the needs of the Kibana page bundle. It's not clear that your plugin's setup affects the bundle... but we're actively driving down the size of the bundle, as well, for performance concerns. So there's an explicit (and sometimes overwhelming) win to the ecosystem when a plugin "sets something up" on mount.

I don't think we can argue that adding hundreds of kb to the page bundle is a winning trade-off for plugin lifecycle purity. We either need to strike a balance, provide lifecycle hooks that allow for it, or refine the philosophy... or all of the above.

[clintandrewhall]

Something else to consider, and why I'd be a fan of discussing expanding the plugin lifecycle... Canvas has no dependents, (at the moment). We expose nothing of use or to be used by other plugins, and yet our setup code-- all 500k of it-- is added to the page bundle, code that is only used in the Canvas UI after Canvas loads.

So we're likely violating this premise as of #104264 ... I've been experimenting with async loading of function bodies, but it still has a large impact on the page bundle, (see #104990)

So it might make sense to consider a lifecycle option that uses setup functionality safely on mount. At the same time, I'm not a wonk in this respect... but I do have to answer for the code we add to the bundle.

Perhaps it would be useful to treat Canvas as a case study in lifecycle use and optimization? I'd be happy to be the guinea pig for this, particularly if it will help other plugins do the same!

(cc: @spalger and @tylersmalley)

[lukeelmers]

Should accessing setup functionality after the setup lifecycle completes be considered an anti-pattern?

IMO the answer to this is yes. The whole point of the lifecycles is to provide plugins with guarantees as to when they can expect setup tasks to be complete. If we start performing these tasks whenever we want to, it makes Kibana less predictable and therefore less stable.

I think we should see if the plugin system itself can detect this and fail, by wrapping plugin lifecycle methods and failing if they are called after the setup lifecycle completes.

This is an interesting idea and I think it would be possible, but we'd need to spend some time discussing how this would look in practice. cc @elastic/kibana-core WDYT?

I'd love further clarification on what constitutes setup vs start and mount

• setup is for any, well, setup tasks your plugin needs to perform: adding items to registries, registering routes, or other logic that you want to have assurance is completed before performing any actions in start
• start is what most plugins should default to: among other things, it's when you can be guaranteed that all registries are "full" and won't have more items added to them
• If your plugin has registered an application, mount is called when the user navigates to the route for your app. It really has nothing to do with plugin lifecycles.

I don't think we can argue that adding hundreds of kb to the page bundle is a winning trade-off for plugin lifecycle purity. We either need to strike a balance, provide lifecycle hooks that allow for it, or refine the philosophy... or all of the above.

I think this is creating a false dichotomy: This is not a choice between larger bundles or more lifecycle methods. In most cases (including Canvas) the root cause of the large bundle sizes is a large number of items that need to be registered to a shared service during setup. Overall I would say that this isn't a problem with our lifecycle design, but rather with our registry design.

As I've mentioned in the Expression leasing PR and a few other places, I think one way to solve this problem is by revisiting how we design our registries. If we build more registries to be async, you would be able to register a promise that can lazily import a registry item. The app services team has an issue discussing this here: #65993

[kobelb]

Should accessing setup functionality after the setup lifecycle completes be considered an anti-pattern?

👍

If so, how can we ensure devs don't do this accidentally?

I defer to the Kibana Core team on the implementation details of this.

[clintandrewhall]

@lukeelmers to clarify, I meant to make the differences between setup, start and mount in docs and examples, perhaps even in audits. :-)

I can certainly take action as stated in #105164 (comment) to absolve Canvas of the anti-pattern. But the idea that it's setup or nothing-- when some setup tasks are only useful when a plugin is loaded-- is still kind of gnawing at me. I'm sure it's because Canvas has no dependents in Kibana, and so burdening it with some of the setup of Canvas feels unnecessary.

[lukeelmers]

@lukeelmers to clarify, I meant to make the differences between setup, start and mount in docs and examples, perhaps even in audits. :-)

Ah, thanks for the clarification. Totally agreed 👍

Regardless of how/if core starts enforcing this in the plugin system, we should clarify these points in the documentation as part of the scope of this issue.

[clintandrewhall]

the root cause of the large bundle sizes is a large number of items that need to be registered to a shared service during setup. Overall I would say that this isn't a problem with our lifecycle design, but rather with our registry design.

I think my concern is: do these need to be registered globally in Kibana? The answer in our case is no... in Canvas, we have no need or desire for these functions to be used anywhere else, and protecting those constructs from use elsewhere is no different than not exporting code from public/index.ts.

As @lukeelmers pointed out, we can simply fork the Expressions service and continue to isolate these functions, (and give us the flexibility to deprecate and change them as we see fit)... as one option.

But if the lifecycle dictates that the fork() and registration must happen in setup-- and thus the code is a part of the page bundle-- we still have a problem: loading code no one else needs every time Kibana is loaded in the browser.

[mshustov]

If we agree on 1. we can add docs for 2, but I don't think docs are sufficient. I think we should see if the plugin system itself can detect this and fail, by wrapping plugin lifecycle methods and failing if they are called after the setup lifecycle completes.

I've created #66366 to enforce separation between the lifecycles, but we've never had time to work on it. Should we move this discussion to #66366?

I'd love further clarification on what constitutes setup vs start and mount

Let us know what we can improve in the lifecycle docs https://www.elastic.co/guide/en/kibana/current/kibana-platform-plugin-api.html#plugin-lifecycles
It doesn't contain mount description though, because it is not a lifecycle.

This is an interesting idea and I think it would be possible,

IIRC we discussed using Proxy in dev mode to detect API access in a wrong lifecycle method.

[clintandrewhall]

I've created #105675 to track us fixing the Canvas Plugin inclusion of this anti-pattern.

As it turns out, the Canvas plugin lifecycle is kind of tangled up at the moment, but things are also poorly-named. I think a lot of this is left over from not only the New Platform migration, but also from Canvas being conceived as a stand-alone plugin, outside the Kibana repo. So there's a lot to unpack and untangle.

Canvas uses the expressions plugin as both a setup and start dependency, but in reality, all it needs to do is call fork() in ExpressionsPluginStart and provide the ExpressionsService to our already-defined internal service. That would remove our dependency on both the start and setup lifecycle to define our registries, Expressions and the rest.

From there, it should be trivial to decide how to remove the call to fork() in Canvas as we decide how to handle private/protected/public Expression functions.

Be aware: Lens would be subject to failure under this new rule, but I think their fix would be just as easy if they either 1/ consider a fork or 2/ create a service to encapsulate Expressions from ExpressionsPluginStart instead of setup, or 3/ something else.

So I doubt I have further objection to this kind of guard. But honestly, I'd prefer education to failure, but I leave that to Kibana Core, (like @kobelb so wisely suggested) :-D

[kobelb]

@clintandrewhall /s/wisely/lazily

[timroes]

I would second what Stacey and Luke said here, we should make a clear separation between setup calls and later phases and should consider this an anti-pattern we don't want to use. This would just cause hard to find timing issues later potentially or indeterministic behavior. That this currently blows our page load bundle side is .. well not ideal, but I'd second Luke here, this is not a problem of the lifecycle, but a problem of how we design our registries.

If we know the object registered into them is already large, we should by default design them as accepting a callback function returning a promise, so we can async load that code when the library is accessed not when registered. Given that a lot of our registries are around for some time (longer than the Kibana Platform) that would def need some refactoring, but I think it's the better way.

There might also be other issues (like in this Canvas) example, that are not directly linked to the registry itself, but the overall problem, that Clint mentioned that we need to register items we only need within a plugin in a global registry in another plugin. But also in this case I think we should strive for the right solution then, and make sure that if we consider those to be absolute private to Canvas, they should never need to be registered outside, and basically be passed into the expression executor as a "local registry" solely maintained within Canvas (in which case the page laod bundle size problem) would neither exist.

The Lens change above might btw be the best example why calling those async is extremly dangerous. We registered those functions to a global registry and potentially (even without exact knowledge of anyone within the Lens team) they could have been consumed and used in other parts too, directly from the registry. Now switching it over to async loading could break those usage unintentionally. As far as I am aware this did not happen anywhere, but it's def one of those pitfalls why I think using this anti-pattern is not a real solution, but properly thinking about async registries or local registries where more applicable should be the solution to this.

Some similar discussion about having a shared registry implementation that would allow exactly those "freezing" after setup phases already came up in #36705 (just for reference).

[mshustov]

We are going to start with logging a warning whenever lifecycle API is called outside of the current lifecycle timeframe #124039