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

fix: ipv6 localhost #288

Closed
wants to merge 4 commits into from
Closed

fix: ipv6 localhost #288

wants to merge 4 commits into from

Conversation

chenxsan
Copy link

@chenxsan chenxsan commented Dec 27, 2020

There seems to be a bug in the deprecated url.format() nodejs/node#36654 when formatting ipv6 url:

const url = require('url')
console.log(url.format({
    protocol: 'http',
    hostname: '[::]'
}))

We'll get result of http://[[::]]. Same behavior applies to native-url.

Therefore new SocketClient() would run with url http://[[::]]:8080/sockjs-node when hostname is [::], and that is wrong.

Although they're going to fix it on the Node.js side, but I think it would still make sense to include a quick fix here as it would take some time for native-url to update.

@pmmmwh
Copy link
Owner

pmmmwh commented Dec 30, 2020

Hey, thanks for the PR! We have had our fair share of "bugs" from native-url for a while now that I'm considering to actually roll our own URL formatting utility (it is not that difficult given that all the heavy lifting had been done by the URL implementation).

I'll leave this open while I decide (and maybe work on that).

Reference route that we could take from Node.js: nodejs/node#28482

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.

2 participants