[SIEM] NP Plugin dependency cleanup#64842
Conversation
We are only importing static code from these plugins, and not consuming their plugin contracts. For this reason, we can safely remove them from kibana.json as that imported code will always be included in our own bundle.
If it's not enabled, we simply use a noop for our tracker call.
These are only needed when we're actually using them in our codebase. For request handler contexts, we only need our kibana.json declaration.
|
Pinging @elastic/siem (Team:SIEM) |
| _track = usageCollection?.reportUiStats.bind(null, appId) ?? noop; | ||
| } catch (error) { | ||
| // ignore failed setup here, as we'll just have an inert tracker | ||
| _track = noop; |
There was a problem hiding this comment.
Can this part of the exception be hit anymore? It doesn't seem like it with the way you changed it above. Just a question I have.
There was a problem hiding this comment.
I had a similar thought! Probably not, unless reportUiStats wasn't a function? I debated between the safety of the try/catch vs the cleanliness of not having it.
| export interface StartPlugins {} | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-empty-interface | ||
| export interface PluginSetup {} |
There was a problem hiding this comment.
You can optionally remove these two empty interfaces if you want and just do this below:
export class Plugin implements IPlugin<void, void, SetupPlugins, StartPlugins> {And then remove the return {} you have in the start and setup to not return anything unless I'm missing something and we are returning empty objects for a purpose. But it looks like the contracts allow void for return types and we don't have anything useful to return at the moment.
There was a problem hiding this comment.
@FrankHassanabad we return {} instead of undefined to allow consumers to distinguish between a disabled plugin and an enabled plugin with no contract. We first encountered this with our newsfeed integration, which resulted in #56236.
FrankHassanabad
left a comment
There was a problem hiding this comment.
LGTM and thanks for the interesting and detailed read in the conversation notes as that helps a lot to understand what is going on server side. 👍
With the addition of the null coalescing, the only chance for an error is in usageCollection. However, if the usageCollection contract changes, we should get a type error long before we see a runtime error.
* passes missing generic arguments to public plugin interface * passes missing generic arguments to both plugins' CoreSetup types
Instead, import them from core as needed.
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* Remove static src dependencies from kibana.json We are only importing static code from these plugins, and not consuming their plugin contracts. For this reason, we can safely remove them from kibana.json as that imported code will always be included in our own bundle. * Make usageCollection an optional dependency If it's not enabled, we simply use a noop for our tracker call. * Remove unused plugin contracts These are only needed when we're actually using them in our codebase. For request handler contexts, we only need our kibana.json declaration. * Remove unnecessary try/catch With the addition of the null coalescing, the only chance for an error is in usageCollection. However, if the usageCollection contract changes, we should get a type error long before we see a runtime error. * Improve typings of our Plugin classes * passes missing generic arguments to public plugin interface * passes missing generic arguments to both plugins' CoreSetup types * Don't re-export core types Instead, import them from core as needed.
|
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
* Remove static src dependencies from kibana.json We are only importing static code from these plugins, and not consuming their plugin contracts. For this reason, we can safely remove them from kibana.json as that imported code will always be included in our own bundle. * Make usageCollection an optional dependency If it's not enabled, we simply use a noop for our tracker call. * Remove unused plugin contracts These are only needed when we're actually using them in our codebase. For request handler contexts, we only need our kibana.json declaration. * Remove unnecessary try/catch With the addition of the null coalescing, the only chance for an error is in usageCollection. However, if the usageCollection contract changes, we should get a type error long before we see a runtime error. * Improve typings of our Plugin classes * passes missing generic arguments to public plugin interface * passes missing generic arguments to both plugins' CoreSetup types * Don't re-export core types Instead, import them from core as needed. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
Summary
Addresses #59250.
esUiShared,kibanaUtils)usageCollectionoptionalThe following is an audit of our remaining required dependencies:
alertingactionsalerting.dataembeddablefeatureshomemanagementbesidesinspectorEmbeddablePanellicensingmapstriggers_actions_uiuiActionsEmbeddablePanelChecklist
For maintainers