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

Remove double brackets from the ws url when using raw IPv6 address #2951

Merged

Conversation

ilkkao
Copy link
Contributor

@ilkkao ilkkao commented Dec 29, 2020

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

TBD

Motivation / Use-Case

Fixes: #2943

Breaking Changes

Additional Info

// Need to remove those as url.format blindly adds its own set of brackets
// if the host string contains colons. That would lead to non-working
// double brackets (e.g. [[::]]) host
host = host.replace(/^\[(.*)\]$/, '$1');
Copy link
Member

Choose a reason for hiding this comment

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

Just interesting where we got extra brackets?

Copy link
Contributor Author

@ilkkao ilkkao Dec 29, 2020

Choose a reason for hiding this comment

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

In the problem case browser is navigated to http://[::]:8080/ Websocket URL is formed by taking the current location.hostname which includes the brackets. Then in the end native URL.format method at least on Chrome V8 adds a new set of braces even they already exists.

Feels that this is a V8 URL.format issue and gets someday fixed there based on nodejs/node#36654

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Will be great to add tests, bug not high priority now

@ilkkao
Copy link
Contributor Author

ilkkao commented Dec 29, 2020

Tested manually as I wasn't able to quickly figure out how to add a new snapshot test.

http://[::]:8080/ url is used by the default on my Mac that has IPv6 enabled with the lastest webpack-dev-server beta. That makes this little more visible problem.

@ilkkao ilkkao changed the title Remove double braces from the ws url when using raw IPv6 address Remove double brackets from the ws url when using raw IPv6 address Dec 29, 2020
@ilkkao ilkkao force-pushed the ilkkao/fix-double-braces-in-url branch from 6f51d10 to edb079a Compare December 29, 2020 18:07
@codecov
Copy link

codecov bot commented Dec 29, 2020

Codecov Report

Merging #2951 (03b4a05) into master (3ac015b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2951   +/-   ##
=======================================
  Coverage   92.39%   92.39%           
=======================================
  Files          37       37           
  Lines        1262     1263    +1     
  Branches      326      327    +1     
=======================================
+ Hits         1166     1167    +1     
  Misses         91       91           
  Partials        5        5           
Impacted Files Coverage Δ
client-src/default/utils/createSocketUrl.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ac015b...55c91b2. Read the comment docs.

@ilkkao ilkkao force-pushed the ilkkao/fix-double-braces-in-url branch from edb079a to cca1c7a Compare December 30, 2020 17:11
@ilkkao
Copy link
Contributor Author

ilkkao commented Jan 19, 2021

Test failure btw doesn't seem to relate to my changes.

@thijstriemstra
Copy link

thijstriemstra commented Jan 25, 2021

http://[::]:8080/ url is used by the default on my Mac that has IPv6 enabled with the lastest webpack-dev-server beta. That makes this little more visible problem.

Running into same issue on Ubuntu 20:

webpack 5.17.0 compiled successfully in 1183 ms
<i> [webpack-dev-server] Project is running at http://[::]:8080/

This PR didn't seem to help and it still prints http://[::]:8080/...

@alexander-akait
Copy link
Member

Time to merge and move to new release

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.

[v4.3.0] auto-detected local server url is broken
3 participants