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

Reconcile tree routing handler with workspaces. #4708

Merged
merged 11 commits into from Jun 12, 2018

Conversation

@afshin
Copy link
Member

@afshin afshin commented Jun 11, 2018

Fixes #4502

  • Tree URLs in the default workspace work as before: http://localhost:8888/lab/tree/path/to/notebook.ipynb
  • Tree URLs in a named workspace look like this: http://localhost:8888/lab/workspaces/foo/tree/path/to/notebook.ipynb
  • The reset query string parameter outranks the clone parameter. If both reset and clone exist in the URL, then reset will first blow away the state database and then clone will copy the contents of a different workspace.
  • Both reset and clone outrank the tree routing, meaning their effects will manifest first and then the tree URL will be loaded.
  • reset and clone play nicely with named workspaces as well as the default workspace
  • reset and clone place nicely with tree URLs: after the state database is cleared out, the tree URL is loaded.

The points above mean every URL below should work in a deterministic way that users can reason about:

  • http://localhost:8888/lab/tree/path/to/notebook.ipynb load a tree URL into the default workspace
  • http://localhost:8888/lab/workspaces/foo/tree/path/to/notebook.ipynb load a tree URL into the foo workspace, even if it doesn't already exist
  • http://localhost:8888/lab/workspaces/bar/tree/path/to/notebook.ipynb load a tree URL into the bar workspace, even if it doesn't already exist
  • http://localhost:8888/lab/workspaces/bar/tree/path/to/notebook.ipynb?clone clone the contents of the default workspace into the bar workspace and then load a tree URL
  • http://localhost:8888/lab/workspaces/bar/tree/path/to/notebook.ipynb?clone=foo clone the contents of the foo workspace into the bar workspace and then load a tree URL
  • http://localhost:8888/lab/workspaces/bar/tree/path/to/notebook.ipynb?clone=foo&reset reset the contents of the bar workspace (this is superfluous immediately before a clone, but not a problem), clone the contents of the foo workspace into the bar workspace and then load a tree URL
  • http://localhost:8888/lab/workspaces/bar/tree/path/to/notebook.ipynb?reset reset the contents of the bar workspace and then load a tree URL
  • http://localhost:8888/lab/tree/path/to/notebook.ipynb?reset reset the contents of the default workspace and then load a tree URL
@afshin afshin self-assigned this Jun 11, 2018
@afshin afshin force-pushed the tree-handler-revisited branch from ea8d7ab to a800c26 Jun 11, 2018
@afshin afshin changed the title [wip] Reconcile tree handler routing with workspaces. Reconcile tree handler routing with workspaces. Jun 11, 2018
@afshin afshin changed the title Reconcile tree handler routing with workspaces. Reconcile tree routing handler with workspaces. Jun 11, 2018
Copy link
Member

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

Looks good, works well. I have a few minor questions, but I think this is great.

// will short-circuit and ask the user to navigate away.
const workspace = resolver.name ? `"${resolver.name}"` : '[default: /lab]';

console.log(`Starting ${main.id} in workspace ${workspace}`);
Copy link
Member

@ian-r-rose ian-r-rose Jun 11, 2018

Choose a reason for hiding this comment

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

Do you intend this console.log?

Copy link
Member Author

@afshin afshin Jun 11, 2018

Choose a reason for hiding this comment

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

Yeah, I wanted a concrete use of the IWindowResolver so that it isn't just being included for its side-effects. I'm not sure about this, what's your opinion?

Copy link
Member

@ian-r-rose ian-r-rose Jun 11, 2018

Choose a reason for hiding this comment

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

I think it's reasonable, but I view the plugin ids as being more developer-facing implementation details that should not necessarily be displayed to the user (as opposed to workspace names). Maybe just

console.log(`Starting workspace ${workspace}`)

?

title: 'Build Failed',
body: (<pre>{err.message}</pre>)
});
showDialog({ title: 'Build Failed', body: (<pre>{err.message}</pre>) });
Copy link
Member

@ian-r-rose ian-r-rose Jun 11, 2018

Choose a reason for hiding this comment

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

As long as you are converting things to showErrorMessage, how about this one?

Copy link
Member Author

@afshin afshin Jun 11, 2018

Choose a reason for hiding this comment

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

Good idea, I'll do that.

const url = path + URLExt.objectToQueryString(query) + hash;
const cleared = commands.execute(CommandIDs.recoverState)
.then(() => router.stop); // Stop routing before new route navigation.

// After the state has been reset, navigate to the URL.
cleared.then(() => { router.navigate(url, { silent: true }); });
if (clone) {
cleared.then(() => { router.navigate(url, { silent, hard }); });
Copy link
Member

@ian-r-rose ian-r-rose Jun 11, 2018

Choose a reason for hiding this comment

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

In this branch is the splash not disposed?

Copy link
Member Author

@afshin afshin Jun 11, 2018

Choose a reason for hiding this comment

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

That's right. It's not disposed because the hard navigation means a reload.

Copy link
Member Author

@afshin afshin Jun 11, 2018

Choose a reason for hiding this comment

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

The visual effect I was going for was the splash page still existing as the page unloads. If you look closely, you'll see the splash start over after the refresh, but I think it's still less jarring than the splash disappearing and then reappearing.

@afshin afshin force-pushed the tree-handler-revisited branch 2 times, most recently from 8f07693 to 42c906b Jun 11, 2018
@afshin afshin force-pushed the tree-handler-revisited branch from 42c906b to afbd2ff Jun 11, 2018
@ian-r-rose
Copy link
Member

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

LGTM, thanks!

@ian-r-rose ian-r-rose merged commit f1ff2b5 into jupyterlab:master Jun 12, 2018
2 checks passed
@jasongrout jasongrout added this to the Beta 3 milestone Jun 12, 2018
@afshin
Copy link
Member Author

@afshin afshin commented Jun 12, 2018

cc: @ellisonbg @fperez @choldgraf @willingc @jzf2101

I think you all use this feature sometimes, so I wanted to tag you in this issue so you have access to the list of URL types above.

@willingc
Copy link
Contributor

@willingc willingc commented Jun 12, 2018

Thanks @afshin. It would be great to have a table of the eight examples that you give.
cc/ @minrk

@jzf2101
Copy link
Contributor

@jzf2101 jzf2101 commented Jun 12, 2018

We should figure out how to document this for binder @willingc @minrk

@choldgraf
Copy link
Contributor

@choldgraf choldgraf commented Jun 12, 2018

duuuude so cool, nicely done. Documenting this will indeed be tricky, and probably most-effective with an interactive demo

@jzf2101
Copy link
Contributor

@jzf2101 jzf2101 commented Jun 20, 2018

Is this released yet? I'm trying to run on binder and it's not working yet.

@afshin
Copy link
Member Author

@afshin afshin commented Jun 20, 2018

@jzf2101 This PR is only available in master. We have two more open PRs that we need to merge before the pre-release.

@afshin afshin deleted the tree-handler-revisited branch Jun 20, 2018
@matteoipri
Copy link

@matteoipri matteoipri commented Jun 25, 2018

Hi all,
This morning I tried compiling JupyterLab from source following the instructions here.
I installed it in a Docker image derived from an official image defined in jupyter/docker-stacks.
I am not able to open a file or folder by URL.
Am I missing something?
Thanks

@afshin
Copy link
Member Author

@afshin afshin commented Jun 25, 2018

Hi @matteoipri, how do you start your JupyterLab instance? If you install from source, the only way to get the functionality in master (as opposed to our latest released package) is to run jupyter lab --dev-mode, which means you should be seeing a red bar across your JupyterLab UI:

screen shot 2018-06-25 at 17 10 16

@matteoipri
Copy link

@matteoipri matteoipri commented Jun 25, 2018

Oh, I see. I do not start JupyterLab in dev-mode. I start it from JupyterHub specifying the environment variable, thus with "jupyter labhub". I'll study how to add that optional argument to the launch command.
Maybe it will be easier for me to wait for the release... What is the pre-release? What are the PRs needed to be merged you mentioned some days ago?

@afshin
Copy link
Member Author

@afshin afshin commented Jun 26, 2018

This is the Beta 3 milestone: https://github.com/jupyterlab/jupyterlab/milestone/12

We don't plan to complete all of those before the pre-release. We primarily want these two PRs in, though:

In addition, we want to start automatically formatting our code, so after those two PRs, we plan to also merge this one:

@afshin
Copy link
Member Author

@afshin afshin commented Jul 2, 2018

Fixes #3673

@afshin afshin mentioned this pull request Jul 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.

7 participants