Dynamic extensions reloading

[fcollonval]

Fixes #278

Code changes

There are three additions to the API :

• Application.unregisterPlugin() method to remove the plugin from the plugin map and complement the existing registerPlugin()
• an optional deactivate method which plugins may decide to implement. If it is provided, it should undo all the changes the activate method made, including removing the added commands, menus, etc. to complement the current activate() method.
• Application.deactivatePlugin method to call the deactivate method on the plugin and all of it dependants

The unregisterPlugin signature is:

/**
 * @param id - id of the plugin to unregister
 * @param force - whether to unregister even if active
 */
async unregisterPlugin(id: str, force?: boolean)

deactivatePlugin reject if plugin or one of its dependents does not have deactivate() method:

/**
 * Will deactivate the plugin and its dependant if and only if the plugin
 * and its dependants all support `deactivate`.
 * @returns other dependant plugins deactivated as a consequence
 */
deactivatePlugin(id: string): Promise<string[]>

The return object is a list of plugin ids rather than a IPlugin list to avoid security hole that returning IPlugin will open (the activate function could be altered by deactivating and reactivating a Plugin with the proposed API in #278)

It is up to plugin authors to adopt the deactivate method. This is a backwards-compatible change.

The signature of deactivate is:

deactivate?: (app: T, ...args: any[]) => void | Promise<void>; 

⚠️ deactivating a plugin will deactivate all its dependents optional or not. In theory, we could reactivate the one having a optional relationship. I propose to keep it like this and to let sub class application decide what to do - the rational is that we anyway have to modify some API in JupyterLab itself to support dynamic reload.

---

Side effect I replace the use of objects by mapping because deleting a object key in JS is famous to reduce performance.

[fcollonval]

cc @jasongrout @krassowski

[blink1073]

API changes LGTM!

[afshin]

It took me several tries to git checkout this branch name 🤣

git checkout "fix/278-Dynamic-extensions-reloading-\`IPlugindeactivate\`-\`ApplicationregisterPlugin\`-and-\`ApplicationdeactivatePlugin\`-proposal"

[afshin]

I opened a PR against your fork which contains some of the review comments I made above and a few other changes. I didn't want to push directly on your branch in case you are in the middle of something or in case you disagree.

[fcollonval]

It took me several tries to git checkout this branch name rofl

autogenerated name from issue title... sorry about that

I opened fcollonval#5 which contains some of the review comments I made above and a few other changes. I didn't want to push directly on your branch in case you are in the middle of something or in case you disagree.

Thanks a lot for the review and the commit to improve this PR. I have only one blocking comment about your PR regarding not using sequential deactivation of the plugins: https://github.com/fcollonval/lumino/pull/5/files#r956955307

[afshin]

Thank you!

[fcollonval]

@meeseeksdev please backport to 1.x

[lumberbot-app]

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

1. Checkout backport branch and update it.

git checkout 1.x
git pull

2. Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 10c89680d31ce17324736b4737ba3537bdb5cd11

3. You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #377: Dynamic extensions reloading'

4. Push to a named branch:

git push YOURFORK 1.x:auto-backport-of-pr-377-on-1.x

5. Create a PR against branch 1.x, I would have named this PR:

"Backport PR #377 on branch 1.x (Dynamic extensions reloading)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

[fcollonval]

I won't backport it because Map does not exist in older es version.

[jtpio]

I won't backport it because Map does not exist in older es version.

So for reference this seems to have been backported anyway in #485?