Plugin types #40122
Plugin types #40122
Conversation
streamich
added
review
release_note:skip
|
Pinging @elastic/kibana-app-arch |
streamich
added
Team:Core
|
Pinging @elastic/kibana-platform |
💚 Build Succeeded |
src/core/server/plugins/plugin.ts
Outdated
| @@ -168,8 +169,8 @@ export type PluginInitializer< | |||
| export class PluginWrapper< | |||
| TSetup = unknown, | |||
| TStart = unknown, | |||
| TPluginsSetup extends Record<PluginName, unknown> = Record<PluginName, unknown>, | |||
| TPluginsStart extends Record<PluginName, unknown> = Record<PluginName, unknown> | |||
| TPluginsSetup extends EmptyPluginContracts = EmptyPluginContracts, | |||
There was a problem hiding this comment.
I just realised that extends EmptyPluginContracts refers to an empty interface, not object. with this change we have a problem that any real type like number, string, etc. satisfy this conditions. now I think we should rollback to Record<string, unknown> or use object instead
There was a problem hiding this comment.
@restrry
I'm not sure if that is something we should worry about, but if you want to fix that you can do:
type EmptyPluginContracts = {} & object;or use a branded type:
type EmptyPluginContracts = {} & { __brandPluginContract?: 'yup' };or use a branded type from some lib like utility-types:
type EmptyPluginContracts = Brand<{}, 'PluginContracts'>There was a problem hiding this comment.
I'm not sure if that is something we should worry about
yes, right now it doesn't specify a type of a contract at all
TPluginsSetup extends object = {},should be enough
There was a problem hiding this comment.
That does not seem to work.
There was a problem hiding this comment.
hm. it falls back to the empty interface as well. either options should work
TPluginsSetup extends object = object,
TPluginsSetup extends Record<string, unknown> = Record<string, unknown>,There was a problem hiding this comment.
@restrry
After playing with it for a bit, I think I have something that cover "all" 4 use cases:
- Unknown properties cannot be accessed.
- Plugin contract cannot be a primitive "number" type.
- If plugin contract is not specified, it still cannot be a primitive like "number".
- Plugin can have no contract.
There was a problem hiding this comment.
Ah, actually object/object this way seems to cover those cases too:
TPluginsSetup extends (object | void) = objectOK, I know why. Because type NonPrimitive = object, so my NotPrimitive<{}> is basically object. 😄
If you do that same thing with Record it does not work.
TPluginsSetup extends (Record<string, unknown> | void) = Record<string, unknown>There was a problem hiding this comment.
@restrry so, I'm gonna implement
TPluginsSetup extends (object | void) = objectif you are okay with that?
| @@ -117,15 +117,17 @@ test('`setupPlugins` throws if plugins have circular optional dependency', async | |||
|
|
|||
| test('`setupPlugins` ignores missing optional dependency', async () => { | |||
| const plugin = createPlugin('some-id', { optional: ['missing-dep'] }); | |||
| jest.spyOn(plugin, 'setup').mockResolvedValue('test'); | |||
| jest.spyOn(plugin, 'setup').mockResolvedValue({ foo: 'bar' }); | |||
There was a problem hiding this comment.
TypeScript found these "errors" in tests—plugin contract must be an object.
💚 Build Succeeded |
| @@ -20,7 +20,7 @@ | |||
| import { PluginInitializerContext, CoreSetup, CoreStart, Plugin } from '../../../core/public'; | |||
| import { ExpressionsService } from './expressions/expressions_service'; | |||
|
|
|||
| export class DataPublicPlugin implements Plugin<{}> { | |||
| export class DataPublicPlugin implements Plugin<ReturnType<DataPublicPlugin['setup']>, void> { | |||
There was a problem hiding this comment.
Just a note that I'm working on some conventions for the plugin files here which would include exporting a dedicated DataSetup interface that you could pass to this generic: #39554
Would be interested in your thoughts on that PR if you have time to review!
| /** | ||
| * Custom window type for loading bundles. Do not extend global Window to avoid leaking these types. | ||
| * @internal | ||
| */ | ||
| export interface CoreWindow { | ||
| __kbnNonce__: string; | ||
| __kbnBundles__: { | ||
| [pluginBundleName: string]: UnknownPluginInitializer | undefined; | ||
| [pluginBundleName: string]: object | undefined; |
There was a problem hiding this comment.
Not sure it's a correct.__kbnBundles__ contains init functions, not objects.
https://github.com/restrry/kibana/blob/af295b43bdf27bcefd7ed293fa97e96a9656add0/src/core/public/plugins/plugin_loader.ts#L91-L99
| [pluginBundleName: string]: object | undefined; | |
| [pluginBundleName: string]: PluginInitializer | undefined; |
There was a problem hiding this comment.
That actually illustrates another problem with the current code. any not primitive type is a valid one
Summary
Adds
EmptyPluginContractsdefault empty plugin contract type.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.This was checked for cross-browser compatibility, including a check against IE11Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsUnit or functional tests were updated or added to match the most common scenariosThis was checked for keyboard-only and screenreader accessibilityFor maintainers
This was checked for breaking API changes and was labeled appropriatelyThis includes a feature addition or change that requires a release note and was labeled appropriately