Skip to content

switch default ws_url logic to js side#3307

Merged
Carreau merged 2 commits into
ipython:masterfrom
minrk:wsproto
Jun 2, 2013
Merged

switch default ws_url logic to js side#3307
Carreau merged 2 commits into
ipython:masterfrom
minrk:wsproto

Conversation

@minrk
Copy link
Copy Markdown
Member

@minrk minrk commented May 11, 2013

In some cases (proxies, #3305), the request object doesn't have the right information about the originating information. This changes the default behavior, so that ws_url is generally empty by default, which the javascript takes to mean 'the same as http'. This is simpler and should be more resilient than trying a guess on server-side.

also replaces unused websocket_host with websocket_url

Rather than specifying only the hostname, it makes much more sense to specify the whole protocol,host,port in a single go.

@Cartroo
Copy link
Copy Markdown

Cartroo commented May 11, 2013

Tested using nginx as a HTTPS reverse-proxy for a HTTP ipython notebook server and it all seems to work fine. I haven't done particularly extensive testing, but I can edit and execute code successfully.

@Carreau
Copy link
Copy Markdown
Member

Carreau commented May 19, 2013

Looks fine to me, but need rebase right now. Will probably also conflict with @ellisonbg refactor of JS.

@minrk
Copy link
Copy Markdown
Member Author

minrk commented May 19, 2013

almost certainly - I will rebase after that one gets merged.

minrk added 2 commits May 29, 2013 15:53
In some cases (proxies, ipython#3305), the request object doesn't have the right information about the originating information.  This changes the default behavior, so that `ws_url` is generally empty by default, which the javascript takes to mean 'the same as http'.  This is simpler and should be more resilient than trying a guess on server-side.
Rather than specifying only the hostname, it makes much more sense
to specify the whole protocol,host,port in a single go.
@minrk
Copy link
Copy Markdown
Member Author

minrk commented May 29, 2013

rebased

@Carreau
Copy link
Copy Markdown
Member

Carreau commented May 30, 2013

+1

@Carreau
Copy link
Copy Markdown
Member

Carreau commented Jun 1, 2013

Merging soon if no objections.

@Carreau
Copy link
Copy Markdown
Member

Carreau commented Jun 2, 2013

Oups, I forgot to merge this one.
Merging.

Something wrong we are dangerously approaching <10 opened PRs...

Carreau added a commit that referenced this pull request Jun 2, 2013
switch default ws_url logic to js side

In some cases (proxies, #3305), the request object doesn't have the right information about the originating information. This changes the default behavior, so that ws_url is generally empty by default, which the javascript takes to mean 'the same as http'. This is simpler and should be more resilient than trying a guess on server-side.

also replaces unused websocket_host with websocket_url

Rather than specifying only the hostname, it makes much more sense to specify the whole protocol,host,port in a single go.
@Carreau Carreau merged commit 3c2f44d into ipython:master Jun 2, 2013
@tkf
Copy link
Copy Markdown
Contributor

tkf commented Jun 10, 2013

It would be nice if Dev: URL mapping of IPython notebook were included in sphinx document, had more specific document about JSON content, and updated when this patch is applied. It causes a problem in Emacs client and I needed sometime to figure out what's going wrong. Of course you don't need care about Emacs client, but documenting this kind of stuff makes project better, I think.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am 90% sure this is a dumb question, but can't location.origin start with "https"?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes - this will turn http:// into ws:// and https:// into wss://, which is how you specify "websocket over https"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, I see. You are using s from https.

@Carreau
Copy link
Copy Markdown
Member

Carreau commented Jun 11, 2013

Just to pitch un à little late, sorry @tkf, I would have seen this would
break EIN I would have pinged you (was probably tired, because seem obvious
after the fact). We do care about compatibility with EIN, and at least give
you and other project some time to adapt when we make changes.

Le lundi 10 juin 2013, Takafumi Arakaki a écrit :

It would be nice if Dev: URL mapping of IPython notebookhttps://github.com/ipython/ipython/wiki/Dev%3A-URL-mapping-of-IPython-notebookwere included in sphinx document, had more specific document about JSON
content, and updated when this patch is applied. It causes a problem in
Emacs client and I needed sometime to figure out what's going wrong. Of
course you don't need care about Emacs client, but documenting this kind of
stuff makes project better, I think.


Reply to this email directly or view it on GitHubhttps://github.com//pull/3307#issuecomment-19212384
.

@tkf
Copy link
Copy Markdown
Contributor

tkf commented Jun 11, 2013

@Carreau Thanks, pinging me when there is major JS kernel change would be great. I guess it would be nice if automated test can be hooked to upstream change in Travis. But I guess that require changing .travis.yml in IPython and won't scale as there are many IPython-related projects. I am thinking to setup a cron job to watch IPython change.

Regarding the document I opened an issue: #3417

@minrk minrk deleted the wsproto branch March 31, 2014 23:36
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
switch default ws_url logic to js side

In some cases (proxies, ipython#3305), the request object doesn't have the right information about the originating information. This changes the default behavior, so that ws_url is generally empty by default, which the javascript takes to mean 'the same as http'. This is simpler and should be more resilient than trying a guess on server-side.

also replaces unused websocket_host with websocket_url

Rather than specifying only the hostname, it makes much more sense to specify the whole protocol,host,port in a single go.
smoofra pushed a commit to smoofra/emacs-ipython-notebook that referenced this pull request Mar 21, 2015
IPython notebook 1.0.dev sends empty ws_url as of this PR
ipython/ipython#3307
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants