-
Notifications
You must be signed in to change notification settings - Fork 2
chore: resource registry #85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: plugin-manifest-definition
Are you sure you want to change the base?
Conversation
b7f9199 to
ff847ee
Compare
ec151bd to
f3aca2e
Compare
pkosiec
left a comment
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.
Overal LGTM, two small comments - please check them and address them (if applicable) here or in a follow-up PR π
| @@ -15,7 +15,16 @@ interface ReconnectStreamResponse { | |||
|
|
|||
| export class ReconnectPlugin extends Plugin { | |||
| public name = "reconnect"; | |||
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.
BTW - do we need the public name for plugins if it's already in the static manifest? Or is it like an instance name which should vary when you instantiate the class multiple times?
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.
Can we extract resource collection and validation to some separate methods? The logic here a bit makes it harder to read the high-level logic for registering plugins.
Like it was before:
pluginInstance.validateEnv();so now I can imagine, we'd have two calls: validation + collecting resources
π Stack (3/3)
π Changes
b7f9199chore: resource registryπ About this stack
This PR is part of a stack of changes. Each PR in the stack builds on the one below it.
plugin-manifest-definition