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

Clean up handling of default urls #5111

Merged
merged 23 commits into from Aug 14, 2018
Merged

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 12, 2018

Fixes #4761. Cleans up handling of baseUrl and wsUrl in both PageConfig and ServerConnection.

  • urls are now normalized using a new URLExt.normalize function.
  • Do not fall back on http://localhost
  • Clean up the fallback logic for wsUrl
  • makeSettings does not use the baseUrl parameter in PageConfig.getWsUrl since technically that parameter doesn't make sense there, but I did not want to break the API in coreutils
@blink1073 blink1073 added this to the 0.34 milestone Aug 12, 2018
@blink1073 blink1073 added the bug label Aug 12, 2018
@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 12, 2018

@blink1073 blink1073 self-assigned this Aug 13, 2018
@vidartf
Copy link
Member

@vidartf vidartf commented Aug 13, 2018

@blink1073 For me, the most interesting scenario is the behavior when there is not a jupyter-config-data element on the page. As far as I can tell, the changes for that scenario now are:

  • ServerConnection.defaultSettings will have a blank string for baseUrl/wsUrl instead of having it be based on location.
  • Calling ServerConnection.makeSettings() without supplying at least a baseUrl option will throw an error instead of using location.

This means that in order to make code that is agnostic about the page containing a jupyter-config-data element, my current code will have to change as follows:

export
function requestJsonPromise(url: string, argument: any): Promise<JSONObject> {
  let request = {
      method: 'POST',
      body: JSON.stringify(argument),
    };
- let settings = ServerConnection.makeSettings();
+ let baseUrl = ServerConnection.defaultSettings.baseUrl || window.location.origin + '/';
+ let settings = ServerConnection.makeSettings({ baseUrl });
  return ServerConnection.makeRequest(url, request, settings)
    .then(handleError)
    .then((response) => {
      return response.json();
    });
}

Correct? Anything I missed?

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 13, 2018

You'd need to add 8888 to the url get the old behavior.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 13, 2018

Oh wait, I see what you mean about using window.location as a default.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 14, 2018

@vidartf, PageConfig.baseUrl will now fall back on location.origin, meaning the only time you can't get away with not specifying a baseUrl is in node.

: location.origin + '/';
const baseUrl = URLExt.normalize(getOption('baseUrl'));
if (!baseUrl && typeof location !== 'undefined') {
return URLExt.normalize(location.origin);
Copy link
Member

@vidartf vidartf Aug 14, 2018

Choose a reason for hiding this comment

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

What happens if you simply use URLExt.normalize('/') here, and skip the location check? Will this work on node as well?

Copy link
Member Author

@blink1073 blink1073 Aug 14, 2018

Choose a reason for hiding this comment

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

It gives back / in node, which would go to the root of your server. However, that doesn't help us get a wsUrl...

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 14, 2018

@vidartf, thinking more about your use case, I made wsUrl optional altogether and allowed / on node as the default baseUrl.

Copy link
Member

@vidartf vidartf left a comment

Seems good. Only noting that any use of websockets on Node without ensuring a valid wsUrl will now trigger the exception Error: Invalid URL: . That should hopefully be sufficiently clear for Node consumers, together with the traceback.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 14, 2018

Having a default wsUrl in node is odd anyway, since WebSockets are not built in 😜.

@blink1073 blink1073 merged commit 0a0fbf9 into jupyterlab:master Aug 14, 2018
2 checks passed
@blink1073 blink1073 mentioned this pull request Aug 17, 2018
@blink1073 blink1073 deleted the ws-url-fix branch Sep 4, 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.

2 participants