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 Broken Socket on Client for Custom/Random Port Numbers #1060

Merged
merged 2 commits into from Aug 30, 2017

Conversation

shellscape
Copy link
Contributor

What kind of change does this PR introduce?
Bugfix

Did you add or update the examples/?
No need

Summary
#1059 exposed a situation in which the sockjs url for clients with custom/random ports would result in the client using an erroneous url that either omitted the port or used the wrong (default 8080) port. This resulted in socket communication breaking, which was noticed by way of the lack of an overlay being displayed.

This fix does the following:

  1. in Server.js the socket server won't be initialized nor started until the listeningApp triggers the callback on listen. That's really the proper time for the socket server to be initialized anyhow. polyfills dep is also moved to the top to match bin/webpack-dev-server.js.
  2. in client/index.js the port parsing is going to look for null and '0' on urlParts.port and that'll be refactored slightly.
  3. in bin/webpack-dev-server.js the polyfills dep is added to the top of the file preventing internal-ip errors on Node 4.x.

Does this PR introduce a breaking change?
Negatory

Other information

@codecov
Copy link

codecov bot commented Aug 29, 2017

Codecov Report

Merging #1060 into master will decrease coverage by 0.04%.
The diff coverage is 37.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1060      +/-   ##
==========================================
- Coverage   73.37%   73.33%   -0.05%     
==========================================
  Files           5        5              
  Lines         447      450       +3     
  Branches      141      142       +1     
==========================================
+ Hits          328      330       +2     
- Misses        119      120       +1
Impacted Files Coverage Δ
lib/Server.js 80.12% <37.03%> (-0.13%) ⬇️

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 1201ac1...514b188. Read the comment docs.

@shellscape shellscape merged commit 0fa8fea into master Aug 30, 2017
@shellscape shellscape deleted the fix-socket-random-port branch August 30, 2017 01:11
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.

None yet

1 participant