Plugin lifecycle methods that return undefined block dependents from inferring the plugin's disabled state

[cjcenizal]

Plugins that declare other plugins as dependencies will commonly inspect the plugins argument to see if that plugin is enabled or not.

if (plugins.dependencyPlugin !== undefined) {
  // If it's defined, it's enabled.
}

However, plugins have the option of returning no value from their lifecycle methods. In this case, the above condition will never evaluate to true because plugins.dependencyPlugin is undefined. Currently, the only way to fix this is to update the dependent to return something from its lifecycle method. See this PR for an example: https://github.com/elastic/kibana/pull/68919/files#diff-70b0ed13bb3e2d531664f49faf0c804dR28.

This seems like an anti-pattern to me -- plugins shouldn't be able to "hide" their enabled state by returning undefined from their lifecycle methods. At the same time, it seems like unnecessary boilerplate to put the burden on plugin developers to always return empty objects from their lifecycle methods.

I think the simplest solution would be for the Platform to check if a lifecycle method returns undefined, and to provide an empty object to dependents instead. Thoughts?

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[pgayvallet]

I was going to open an issue for this specific problem, as I had a very similar discussion recently here: #69581 (comment)

IMHO, the correct solution, even if it's more impacting, would be to simply forbid void as a valid plugin contract and force the plugins to return an empty object instead.

export interface Plugin<
  TSetup = void,
  TStart = void,
  TPluginsSetup extends object = object,
  TPluginsStart extends object = object
>

would simply become

export interface Plugin<
  TSetup  extends object ={},
  TStart  extends object = {},
  TPluginsSetup extends object = object,
  TPluginsStart extends object = object
>

@elastic/kibana-platform what do the others think?

[mshustov]

@pgayvallet I like your proposal for simplicity. OTOH I'm wondering if it semantically closer to the status service and should be part of it.

[pgayvallet]

You mean the correct way to assert if a plugin is available or not would/should be to use the status service (once #41983 is done), right?

[cjcenizal]

@pgayvallet @restrry I can understand the desire to use TS to force plugin maintainers to do the right thing. From the phrasing of "correct way to assert if a plugin is available", I'm also inferring that we're seeking a canonical way for developers to determine plugin availability, which I can understand.

But I have a strong desire that we consider the developer experience foremost when considering a solution. As a developer, I don't want to have to add an empty return value (and an empty interface!) just to satisfy a TypeScript rule. This would impose significant burden on all Kibana plugin developers to update their plugins to adhere to this rule, for little (zero?) gain to the developers themselves. From a developer's perspective, I worry that it's just a new hoop to jump through. Using the Platform to transparently fix this problem without requiring developers to change plugin code is very attractive to me because it doesn't disrupt plugin developers.

Similarly, I reach for plugins when I want to use a plugin. It's intuitive to infer that if a plugin isn't there (which should only occur when it's disabled), then I can't use it and it's simple to apply this logic to the code. It seems like needless complexity to consume a special service to tell me if a plugin is enabled before I try to use it.

[mshustov]

@pgayvallet right.
@cjcenizal I reasoned that a plugin might be available at the start, but unavailable later due to an error, so you will have to check a dependency availability via status service anyway. Or we just ignore such a possibility at the moment?

[cjcenizal]

@restrry Ah, I see! Thanks for explaining.

I think I'd like to have the error case clearly defined and understood. If we're only speculating at this point, then I think the pragmatic course would be to ignore this possibility. If we were to update plugin code to be robust in the event of any plugin dependency failure, then I think we'll an introduce a non-trivial amount of complexity into the code (try/catches and fallback UI states if functionality is missing, and maybe some way of communicating the abnormal state to the user). Without a defined error case to reproduce in tests, we also won't have confidence that this code is behaving as expected.

Perhaps the simplest solution would then be to say that an unanticipated error in a plugin dependency (or a plugin in general) should be treated as a fatal error by Kibana?

[pgayvallet]

Only my opinion (fundamentally have no real objections against the undefined to {} implicit conversion), but

I can understand the desire to use TS to force plugin maintainers to do the right thing

That's not only that. ATM the contracts returned by the plugins are the exact structures passed down to the dependent plugins. Having an interference, even as basic as depContract ?? {}, kinda go against this implicit logic.

Also as the dependency is atm returning undefined, the dependant's dependency definition is not properly typed

interface MyPluginSetupDeps {
   pluginThatReturnsUndefined: {} // or any, or what?
}

I agree that this is like minor, but still.

As a developer, I don't want to have to add an empty return value (and an empty interface!) just to satisfy a TypeScript rule

We're only talking about adding <{}, {}> in the plugin definition and return {} in two methods here. Don't even necessarily need to add an empty interface, the empty object type is often inlined in these cases atm (Plugin<{}, {}>). That do feels like a very minor impact to me.

Also, regarding

Using the Platform to transparently fix this problem without requiring developers to change plugin code is very attractive to me because it doesn't disrupt plugin developers.

Platform would very likely have to fix all existing violations if implementing this rule anyway, as PR would not pass CI without these fixes. So it would only 'disrupt' new plugins.

[joshdover]

The Platform will only specify a key in the dependencies object if the plugin is enabled, if the plugin is not enabled the key will not be defined at all. This is a subtle behavior but maybe we can leverage this instead:

if ('dependencyPlugin' in plugins) {
  // If the key exists in `plugins`, it's enabled.
}

Using this condition will work regardless of what the plugin returns (even if undefined) and can be properly typed:

// GOOD - using optional keys means the key may not exist at all in `Object.keys(plugins)`
interface MyStartDeps {
  dependencyPlugin?: DependencyStart
}

// BAD - because the key may not always exist (in the case of optional deps)
interface MyStartDeps {
  dependencyPlugin: DependencyStart | undefined;
}

[lukeelmers]

Closing as this has become stale, and #69845 (comment) is the recommended way to handle these scenarios at this point.