-
-
Notifications
You must be signed in to change notification settings - Fork 127
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
Add a 'defer' option to the autoStart argument #588
Conversation
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.
Thanks @brichet
I have some suggestions / questions
packages/application/src/index.ts
Outdated
@@ -59,6 +59,14 @@ export interface IPlugin<T extends Application, U> { | |||
*/ | |||
autoStart?: boolean; |
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.
Could we use autoStart?: boolean | 'defer'
instead of introducing another flag? This will be consistent with the actual three available state.
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.
Sure, it should work without breaking the current API.
packages/application/src/index.ts
Outdated
* @returns A list of promises which will each resolve when the related | ||
* plugin is activated or rejects with an error if it cannot be activated. | ||
*/ | ||
activateDeferredPlugins(): Promise<void>[] { |
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.
Why not wrapping the list of promise in a Promise.all
to get a single promise?
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>
… promise when activating all deferred plugins
Thanks @fcollonval, I updated the PR to include your suggestions. |
This is backward-compatible, so an extension with |
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.
Thanks @brichet
i took the liberty to change slightly the activateDeferredPlugins
to not return an array of void and some technical debts maintenance removing a Map that is used for a Set.
Thanks @fcollonval, I can't remember why I first used a Map for this use case. |
It is not on you it was there from the start of the project 😉 . I think this was used to ensure id uniqueness back in the day where |
* Add an optional 'deferred' to the plugin options * Update packages/application/src/index.ts Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com> * Update packages/application/src/index.ts Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com> * Uses the autostart argument to defer the plugins, and return only one promise when activating all deferred plugins * Fix tests * Don't return void[] from activateDeferredPlugins --------- Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com> Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>
This PR adds an optionaldeferred
flag to the plugin options.EDIT This PR allows to set the value 'defer' to the
autoStart
argument of a plugin.This is related to jupyterlab/jupyterlab#14576.
It also adds a function for activating all deferred plugins, preferably after application startup.
It does not modify the behavior of
autoStart
, which means that ifautoStart: false
, the plugin will not be activated using the dedicated function.