fix hostname resolving on newConnection #384
Conversation
|
The issue affects Electron as well ! |
| var hostname = urlParts.hostname; | ||
|
|
||
| if (urlParts.hostname === '0.0.0.0') { | ||
| if (window.location.hostname && !!~window.location.protocol.indexOf('http')) { |
There was a problem hiding this comment.
Why the check for http here (not saying it is wrong per se)? If it has a valid reason, could you add a comment above? And also please give an example for when the hostname does not exist in the comments.
We get many PR's around the detection of this url, so comments will help other people here.
There was a problem hiding this comment.
for example in electron or any other opened local file in browser, protocol === 'file' and hostname = undefined, hence SockJs url resolves to "http:/sockjs-node" that caused a bug SyntaxError: The URL's scheme must be either 'http:' or 'https:'. 'file:' is not allowed, so we need to be sure that protocol contain 'http' part
There was a problem hiding this comment.
Could you add that to the code as a comment though? This is not immediately clear, and since there are no tests yet, it's easy for this to get accidentally removed.
|
Other then the comment above and the merge conflict, this looks good. Will merge if you finish this :). |
|
Pinging @svsool, are you still interested in finishing this? This would fix a couple of other problems. I could takeover if you don't have the time. |
|
Fix it and create another fresh pull #594 |
correct behaviour when:
I'll get bug with
ionic emulate iosbecause default protocol in WebView is :file and hostname is empty string, hence SockJs url resolve to "http:/sockjs-node" that caused a bugSyntaxError: The URL's scheme must be either 'http:' or 'https:'. 'file:' is not allowed.