This repository has been archived by the owner on Nov 4, 2021. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Plugin Capabilities implementation #384
Plugin Capabilities implementation #384
Changes from 7 commits
7c1fbc2
7804942
bf03525
95670b5
a04a197
7cf3b44
e5bd9d1
063a7e5
8dcb16c
1697073
b2fd679
378f5bc
227aa19
a6837d5
56e0bee
902db6e
f9fc935
c72b680
c1d4d95
8d7f160
caf3cde
56fda74
81bc013
a4ee3ab
f560a8f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if the capabilities haven't changed, there's no need to update them nor log that we set them. That would be needless logspam :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I don't believe these logs are valuable user-side at all. Plugin loaded/unloaded events can be important because of plugin setup/teardown methods – while
capabilities
are just an internal optimization of ours.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could lazy load them. We won't do anything with the loaded capabilities for the moment, and eventually set them in a separate "install" step anyway. No need to block for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I could unblock them, but that still doesn't mean the VMs are lazy loaded right? since
inferPluginCapabilities()
will load the VM on startup, before the worker gets any event.Maybe I should just remove this comment, just realised it's confusing w.r.t what is being lazy loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The funny code here is this:
The first function is run in the background, the second one we await for. The second function awaits for a promise (
resolveInternalVm
) that gets set when the first function that's running in the background finishes.In essence, the plugin is not lazy anymore, but we block until the VM setup has completed.
To get around it, we could just
void
theinferPluginCapabilities
call and let them run in the background, however that also feels slightly hacky.A possibly better idea is to put this
inferPluginCapabilities
inside theLazyVM.initialize
function. There you probably won't needprevConfig
either. Just check if the currentpluginConfig.plugin.capabilities
matches reality or not. If not, update.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that.. makes a lot of sense. Ha, would've been nice if I made this leap myself when thinking about the implementation.
Come to think of it, I don't need the
prevConfig
in the current setup either, for similar reasons - I already have apluginConfig.plugin.capabilities
to check.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here again the
!
could cause subtle issues. After all, we're not sure this function will always be called with pluginConfigs that contain a plugin. It might be the case when we follow the current trail that leads to this function, but it might not be the case in the future.Often the solution is to use a guard, such as
if (!pluginConfig.plugin) { throw new Error('how the hell did you get here?') }
, or justreturn
, depends on the case. Guards are best placed at the top of a function as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a bit quirky that
processEventBatch
is listed as a capability when it's only an autogenerated version based on actually providedprocessEvent
, but that seems OK