Skip to content
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 extension manager #4682

Merged
merged 14 commits into from Jun 29, 2018
Merged

Add extension manager #4682

merged 14 commits into from Jun 29, 2018

Conversation

@vidartf
Copy link
Member

@vidartf vidartf commented Jun 5, 2018

Integrates the code from jupyterlab-discovery (with some cleanup).

@vidartf
Copy link
Member Author

@vidartf vidartf commented Jun 5, 2018

The CI failure is significant, and I see that locally as well. Locally I can work around it by manually building things in this order:

  • coreutils
  • observables
  • services
  • everything

Any ideas?

@vidartf vidartf force-pushed the discovery branch 3 times, most recently from e80fd5b to 918f307 Jun 6, 2018
@vidartf
Copy link
Member Author

@vidartf vidartf commented Jun 6, 2018

The problem was that the code imported from @jupyterlab/services/lib/builder. Now it import from @jupyterlab/services after adding an export of Builder typings namespace in services.

Note: I also added the package to singleton packages, and some deps to vendored, but I just tried to cargo cult, and I have no idea if I did this right.

@vidartf vidartf closed this Jun 6, 2018
@vidartf vidartf reopened this Jun 6, 2018
@vidartf vidartf force-pushed the discovery branch 2 times, most recently from a8d6bc2 to 3063ff3 Jun 8, 2018
@vidartf
Copy link
Member Author

@vidartf vidartf commented Jun 8, 2018

This is now ready for review I think!

@vidartf
Copy link
Member Author

@vidartf vidartf commented Jun 18, 2018

Rebased on latest.

@afshin
Copy link
Member

@afshin afshin commented Jun 21, 2018

There are some issues we need to consider regarding this PR:

  • You can use the discovery UI if you check out and build this branch. However, once you install an extension, you won't be able to use the newly installed extension because you will have been running (future perfect progressive tense) off of the discovery branch in --dev-mode.
  • If you pip install jupyterlab_discovery, you can install this exact functionality into your released version of JupyterLab in order to see it working and use the extensions it installs.
  • This means that if we want to be able to test this functionality in the wild, we'll need it to not simply reside in master (or in discovery) but actually be in a release. This is what @vidartf and I discussed today, and we think that having this extension in the Beta 3 pre-release gives us the ability to get some eyes on the plugin and decide whether to disable it (probably using the setting system and a default value) for the actual Beta 3 release or not. Either way, though, we'd propose shipping this code, but potentially making it disabled (again, I'm using the word "disable" informally, I don't mean disabling via the extension disable mechanism, but by using a setting).
  • One of the potential pitfalls is that a labextension that also has a serverextension component cannot be installed by this current version of the extension. Vidar has experimental code for this in the external version, but it was not ready to put into this PR. The UI of this version will give users a meaningful response if the extension you're trying to install has metadata indicating it has a server extension.

Because this is a big and highly requested feature, I suggest we merge this and put it in the Beta 3 pre-release and iterate on it. There are some UI improvements we've already started talking about (e.g., disabling the "Install" button after it has been clicked, organizing the list of extensions into collapsible sections for those that are install and those that are available, etc.)

cc: @jasongrout @ian-r-rose @ellisonbg

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Jun 21, 2018

I think this is a great enhancement. A couple of thoughts/questions:

  • +1 on including in the prerelease.
  • We have gone to some length to avoid a dependency on node to use the core packages. This would introduce such a dependency. I think that for this reason we should disable it by default, either with the settings system or with the build parameters.
  • In a similar vein, I just tested what happens if you try to use it without node installed, and it does not give a very clear error message. I think we should make it very clear what the problem is if node is not installed.
  • Is there any way to inspect from the package.json of the extension whether it is compatible with the current JupyterLab distribution? That is to say, can it fail before trying to install it?
  • What error will be shown if the user doesn't have write permissions in JUPYTERLAB_DIR?

Thanks for pushing forward on this @vidartf!

@vidartf
Copy link
Member Author

@vidartf vidartf commented Jun 22, 2018

@ian-r-rose I agree that the extension need to handle missing node better. Good point!

Is there any way to inspect from the package.json of the extension whether it is compatible with the current JupyterLab distribution?

This is not a cheap operation, so filtering a search to only compatible versions seem non-trivial. On the server this PR adds some code for checking for an update to any of the installed extensions, which does something similar to that. It could maybe be hinted at with keywords, but I'm not sure if that is a robust solution.

@vidartf
Copy link
Member Author

@vidartf vidartf commented Jun 22, 2018

What error will be shown if the user doesn't have write permissions in JUPYTERLAB_DIR

I haven't tried, but hopefully the same error as the user sees in the terminal (if not, we should ensure that this happens). Whether that error message is currently helpful is another issue.

@jasongrout jasongrout added this to the Beta 3 milestone Jun 27, 2018
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 28, 2018

We had a discussion this morning triaging issues for Beta 3. We thought we should include this PR, but disable the plugin by default so that the stock pip installed jupyterlab does not depend on nodejs. @afshin mentioned writing a setting that disables this plugin by default, but allows the plugin to be enabled.

@vidartf
Copy link
Member Author

@vidartf vidartf commented Jun 29, 2018

@afshin Do you have any recommendation for where to put the enable/disable code, and an outline for how it should look? I've not been tracking the settings system that closely. Also, if this is going to be disabled by default, I guess it makes sense to only have the server extension check for updates on request (now it checks on load for efficiency).

@afshin
Copy link
Member

@afshin afshin commented Jun 29, 2018

Here is how the enabled/disabled status works:

extension-manager-enabled

// If the extension is enabled or disabled, refresh the page.
settings.changed.connect(() => { router.reload(); });

if (!enabled) {
Copy link
Member Author

@vidartf vidartf Jun 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe !== true to guard against other falsy expressions?

Copy link
Member

@afshin afshin Jun 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value should be enforced by the JSON schema, since it's declared as a boolean and since it has a default value. But it doesn't hurt to be explicit. I'll do it in the variable declaration. I'm also thinking of adding an item into the "Settings" menu.

Copy link
Contributor

@jasongrout jasongrout Jun 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should a menu item say something about (requires node)? Or even better, is there some nice error if they don't have node installed? I think we talked about this in our triage - even better would be a good error if a rebuild isn't possible.

I hesitate to make the setting too discoverable until we have a good clear way for a user that doesn't have node installed to know why things don't work.

"properties": {
"enabled": {
"title": "Enabled Status",
"description": "Whether the extension manager is enabled.",
Copy link
Contributor

@jasongrout jasongrout Jun 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should say something about requiring node, I think.

@afshin
Copy link
Member

@afshin afshin commented Jun 29, 2018

@jasongrout What do you think of this:

screen shot 2018-06-29 at 15 19 36

@vidartf
Copy link
Member Author

@vidartf vidartf commented Jun 29, 2018

@afshin I think integrity failure is real (master has moved to TS 2.9.2). Consider re-applying 8212b2a.

@afshin
Copy link
Member

@afshin afshin commented Jun 29, 2018

Ah okay. There was a version mismatch, 2.9.1 vs. 2.9.2. I'll run the integrity utility again.

@afshin
Copy link
Member

@afshin afshin commented Jun 29, 2018

Less horizontal real estate:

screen shot 2018-06-29 at 15 41 28

Copy link
Member

@ian-r-rose ian-r-rose left a comment

This is a monumental piece of work @vidartf, thanks!

I think we should merge this for the RC, and then I encourage everyone on the lab team who has third-party extensions to try to install their updated extensions using this. Hopefully we will be able to find any edge cases that way.

@ian-r-rose ian-r-rose merged commit bf038e4 into jupyterlab:master Jun 29, 2018
2 checks passed
@vidartf vidartf deleted the discovery branch Jul 1, 2018
@ian-r-rose ian-r-rose mentioned this pull request Jul 20, 2018
@vidartf vidartf mentioned this pull request Sep 5, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants