Use location.port when location.hostname is used to infer HMR socket URL #1664

rlamana · 2019-02-13T23:53:49Z

Motivation / Use-Case

On a externally exposed dev server (using host 0.0.0.0), HMR client code builds the socket URL using location.hostname but uses the port set in server's configuration. In a more complex and layered architecture, since the browser is at the very end of it, the proper way to infer socket's URL (as exposed and proxied by dev server in .../sockjs-node/info?t=XX) is to also use location.port along with the location.hostname.

More and more we see the scenario where the dev server is used in a cloud-based development environment or an architecture with some sort of proxy in front of it.

With a dev server externally exposed, host 0.0.0.0 and port 9000 and a proxy exposing it in http://dev.mydomain.com (default port 80).
- Expected HMR socket generated URL: http://dev.mydomain.com/sockjs-node/info?t=XX (default port 80).
- Actual HMR socket generated URL: http://dev.mydomain.com:9000/sockjs-node/info?t=XX (wrong port 9000).

More details in issue: #1663

Breaking Changes

No.
Those who exposed also that wrong port in their proxies as a way around the issue can now remove that configuration.

alexander-akait

Looks good, please add tests

rlamana · 2019-02-15T00:35:32Z

@evilebottnawi How are you testing the logic of the client code? I looked for it but couldn't find it in any of the tests. I guess webdriver is the right solution here but I didn't want to add too much overhead to your current tests. Guide me on how you recommend to do it and I'll be glad to add it.

alexander-akait · 2019-02-15T10:13:34Z

https://github.com/webpack/webpack-dev-server/blob/master/test/Socket.test.js

rlamana · 2019-02-20T17:52:48Z

@evilebottnawi as I understand those tests are checking that the socket and the server are available, but it is not testing the client code that will be executed on the browser's end (more precisely testing the code in https://github.com/webpack/webpack-dev-server/blob/master/client-src/default/index.js).

Any suggestions on how to test it without adding the overhead of webdriver?

https://github.com/webpack/webpack-dev-server/blob/master/test/Socket.test.js

alexander-akait · 2019-02-20T17:54:55Z

/cc @rlamana oh, need thinks how we can tests client, maybe we need setup something to e2e testing
/cc @hiroppy What do you think?

codecov · 2019-02-27T02:44:45Z

Codecov Report

Merging #1664 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1664   +/-   ##
=======================================
  Coverage   83.76%   83.76%           
=======================================
  Files           8        8           
  Lines         536      536           
  Branches      161      161           
=======================================
  Hits          449      449           
  Misses         70       70           
  Partials       17       17

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 cb10f83...cf42f28. Read the comment docs.

rlamana · 2019-02-27T02:52:01Z

@evilebottnawi @hiroppy I added some e2e tests using puppeteer headless Chrome API. This allows testing my fix and enables more e2e testing.

I created a new Client.test.js that contains this client code related tests: https://github.com/webpack/webpack-dev-server/blob/50ef125fbfe04eb51f6788a0c44ada539834ea57/test/Client.test.js

Also the runBrowser helper allows to quickly start the headless browser for e2e testing.

Let me know what you think.

alexander-akait · 2019-03-04T09:14:04Z

Looks good! Ready to merge?

rlamana · 2019-03-04T09:40:02Z

@evilebottnawi it's ready but for some weird reason the test keeps failing in node 6. The proxy I use fails with a EADDRNOTAVAIL error. I've been trying to repro in a container with node 6.16.0 (same version Appveyor uses), but it works just fine and the test passes without problems. Any idea why this could be happening?

alexander-akait · 2019-03-04T09:42:27Z

@rlamana

 console.info node_modules/http-proxy-middleware/lib/logger.js:77
      [HPM] Proxy created: /  ->  http://0.0.0.0:8080
    console.error node_modules/http-proxy-middleware/lib/logger.js:89
      [HPM] Error occurred while trying to proxy request /sockjs-node from localhost:9050 to http://0.0.0.0:8080 (EADDRNOTAVAIL) (https://nodejs.org/api/errors.html#errors_common_system_errors)
    console.error node_modules/http-proxy-middleware/lib/logger.js:89
      [HPM] Error occurred while trying to proxy request /main from localhost:9050 to http://0.0.0.0:8080 (EADDRNOTAVAIL) (https://nodejs.org/api/errors.html#errors_common_system_errors)
  ● Client code › behind a proxy › responds with a 200
    expected 200 "OK", got 500 "Internal Server Error"
      at Test.Object.<anonymous>.Test._assertStatus (node_modules/supertest/lib/test.js:268:12)
      at Test.Object.<anonymous>.Test._assertFunction (node_modules/supertest/lib/test.js:283:11)
      at Test.Object.<anonymous>.Test.assert (node_modules/supertest/lib/test.js:173:18)
      at localAssert (node_modules/supertest/lib/test.js:131:12)
      at node_modules/supertest/lib/test.js:128:5
      at Test.Object.<anonymous>.Request.callback (node_modules/superagent/lib/node/index.js:756:3)
      at IncomingMessage.parser (node_modules/superagent/lib/node/index.js:944:18)
  ● Client code › behind a proxy › requests websocket through the proxy with proper port number
    Timeout - Async callback was not invoked within the 30000ms timeout specified by jest.setTimeout.
      58 |     });
      59 | 
    > 60 |     it('requests websocket through the proxy with proper port number', (done) => {
         |     ^
      61 |       runBrowser().then(({ page, browser }) => {
      62 |         page
      63 |           .waitForRequest((requestObj) => requestObj.url().match(/sockjs-node/))
      at new Spec (node_modules/jest-jasmine2/build/jasmine/Spec.js:92:20)
      at Suite.it (test/Client.test.js:60:5)
      at Suite.describe (test/Client.test.js:42:3)
      at Object.describe (test/Client.test.js:24:1)

Maybe something doesn't handle (like errors) or something starts early then tests

alexander-akait · 2019-03-04T09:57:55Z

Oh, no ideas
/cc @hiroppy

alexander-akait · 2019-03-05T09:28:46Z

Something wrong with CI 😕

rlamana · 2019-03-05T09:41:51Z

@evilebottnawi I'm still trying to figure out why the proxy fails to connect to the devserver on the appveyor Windows environment for node 6 when the host is 0.0.0.0. I tried to launch a local instance of the appveyor env in local to repro the issue and try to debug with no luck...

…OTAVAIL issue

rlamana · 2019-03-05T19:30:58Z

@evilebottnawi @hiroppy, it is now ready to merge.

Apparently, win32/node 6 has issues establishing a connection to 0.0.0.0. I fixed it by just pointing the test proxy to localhost. The test needs the dev server to be listening on 0.0.0.0, it doesn't really matter how the proxy redirects to it anyway, as long as it reaches the dev server.

With this PR you can now use the runBrowser helper I included to add more e2e testing. I hope this helps.

hiroppy · 2019-03-05T21:17:02Z

Sorry for the late reply. Thank you for creating e2e environment.
@rlamana Could you change the port number because 8080 is popular number?

rlamana · 2019-03-05T21:27:14Z

@hiroppy in the test code? sure! I just thought since it should be in an isolated environment it wouldn't matter. I'll just set 9001 for the dev server and 9000 for the proxy.

hiroppy

LGTM if ci is green

hiroppy · 2019-03-06T04:21:31Z

/cc @evilebottnawi PTAL;)

alexander-akait · 2019-03-06T09:28:26Z

test/Client.test.js

+        poll: true,
+      },
+    };
+    addEntries(config, options);


/cc @rlamana Looks we catch bug, when we use API, hot plugin should be added too, can you create issue?

I assumed that it was the API expected behavior, so I didn't think it was a bug. It all makes sense now. I will create the issue then.

I created issue #1703.

alexander-akait

Good work! Let's create issue about note above and we can merge

pedrofurtado · 2019-04-08T20:06:33Z

@rlamana @evilebottnawi This PR, in version 3.3.0, breaks the use of webpack-dev-server with Rails webpacker gem, inside Docker containers. We used to configure rails app to execute in 0.0.0.0:3000 (from container "A"), and webpack-dev-server in 0.0.0.0:3035 (from container "B"). So, the socket URL must be "0.0.0.0:3035". But, with this version, the socket URL is generated as "0.0.0.0:3000" (same port of Rails app, instead of 3035).

pedrofurtado · 2019-04-08T20:07:36Z

The version 3.2.1 does not have problem with it 👍

nick · 2019-04-08T21:01:02Z

This change also broke my setup. I have a static assets server on port 3000 that refers to the webpack-dev-server build on port 8080. Now it is looking for the websocket on port 3000 instead of 8080. This worked fine in 3.2.1.

rlamana · 2019-04-08T22:40:47Z

@pedrofurtado @nick, so as I understand you are not using the dev-server to actually serve your files, but just using it for the hot reload capability?

https://github.com/webpack-contrib/webpack-hot-middleware actually allows you to add hot reloading into an existing server.

rlamana · 2019-04-08T22:57:08Z

I wonder if client-src/default/index.js should also take into account the devServer.public option to override the URL value and support these kind of scenarios where dev server is not use to serve the files.

jcudzich · 2019-04-09T09:05:27Z

@rlamana This change also broke my setup. I using the dev-server the same way as @nick.

chenyong · 2019-04-09T11:01:09Z

This change broke our tools too. We have multiple sites proxied to a single domain with different paths. The old behavior was to get the port the actual Webpack dev server, rather than the one after proxied. Now the port changed and it's very tricky works to update the rest parts of the toolchains.

pedrofurtado · 2019-04-09T11:14:26Z

@pedrofurtado @nick, so as I understand you are not using the dev-server to actually serve your files, but just using it for the hot reload capability?

https://github.com/webpack-contrib/webpack-hot-middleware actually allows you to add hot reloading into an existing server.

Thanks for feedback, @rlamana ! Firstly, thanks for you work in maintainance of webpack-dev-server!

Actually, I use webpack-dev-server for serve static files and hot replacement. For this, we here use the Webpacker gem (Integration between Webpack and Rails -> https://github.com/rails/webpacker ), that provides the two features mentioned (serving static files and hot replacement). Our setup is Rails application running in localhost:3000, and webpack-dev-server (and its socket) running in localhost:3035

My scenario and scenario of @nick are similar 👍

alexander-akait · 2019-04-09T11:19:18Z

It was fixed, please test with latest version

pedrofurtado · 2019-04-09T11:54:50Z

@evilebottnawi Thanks! I will test here. Doubt: The latest version is the 3.3.0 (re-released) or the branch master?

alexander-akait · 2019-04-09T11:56:26Z

@pedrofurtado 3.3.0, but we prepare 3.3.1 on today due some regressions

pedrofurtado · 2019-04-09T11:58:06Z

@evilebottnawi Ok, thank you!

pedrofurtado · 2019-04-10T13:46:26Z

@evilebottnawi @rlamana Even in 3.3.1, the error is still occuring 😢 (as described in #1664 (comment) )

The workaround, for me, is to lock in version 3.2.1 for a while.

hiroppy · 2019-04-10T13:52:25Z

@pedrofurtado Please submit a reproducable repo.

leoliu · 2019-04-12T03:05:12Z

Broke my setup too (similar to #1664 (comment).). webpack-dev-server running on a different port is only used to serve assets i.e. js and whatnot.

alexander-akait · 2019-04-12T10:42:10Z

#1664 (comment)

timboslice69 · 2019-04-16T07:16:38Z

also broke my setup, this fix is a breaking change, maybe revisit it?

alexander-akait · 2019-04-16T10:18:59Z

yep 😞 we have sockPort for this case now #1792, feel free to send a PR with revert

gtzampanakis · 2019-04-17T09:10:32Z

This broke my setup as well. I have an HTML page that references two bundles. Each of them is served from a different WDS instance, running on a different port.

alexander-akait · 2019-04-17T11:18:35Z

PR welcome to revert

rlamana requested review from alexander-akait and michael-ciniawsky as code owners February 13, 2019 23:53

rlamana changed the title ~~Use location.port when location.hostname is used as a fallback in client~~ Use location.port when location.hostname is used to infer HMR socket URL Feb 14, 2019

alexander-akait requested changes Feb 14, 2019

View reviewed changes

rlamana requested a review from hiroppy as a code owner February 27, 2019 02:44

Use location.port when location.hostname is used as a fallback in client

5ecf099

rlamana force-pushed the hmr-use-location-port branch from 41222a3 to 7192e8c Compare February 27, 2019 03:37

Add puppeteer for e2e browser code tests

688f1af

rlamana force-pushed the hmr-use-location-port branch from 4715786 to 688f1af Compare February 27, 2019 04:17

rlamana added 2 commits February 26, 2019 20:36

Fix chokidar issue on test environment

e1f0162

Fix issues with chrome sandbox and proxy unavailable port

d971283

alexander-akait removed the request for review from michael-ciniawsky March 4, 2019 09:13

Use localhost as proxy target in test to avoid appveyor node 6 EADDRN…

9eb19e1

…OTAVAIL issue

rlamana force-pushed the hmr-use-location-port branch from d691605 to 9eb19e1 Compare March 5, 2019 19:05

Change test ports to the 900X range

cf42f28

hiroppy approved these changes Mar 5, 2019

View reviewed changes

alexander-akait reviewed Mar 6, 2019

View reviewed changes

alexander-akait approved these changes Mar 6, 2019

View reviewed changes

rlamana mentioned this pull request Mar 7, 2019

Using API doesn't add HMR plugin entry #1703

Closed

2 tasks

alexander-akait merged commit 2f7f052 into webpack:master Mar 7, 2019

ScottSmith95 mentioned this pull request Apr 9, 2019

[Bug] Local development server broken react-static/react-static#1109

Closed

knagaitsev mentioned this pull request Apr 13, 2019

V3.3.0 live reload broken when set hostname to 0.0.0.0:xxxx #1777

Closed

2 tasks

pedrofurtado mentioned this pull request Apr 16, 2019

feat: add sockPort options in queryString #1792

Merged

6 tasks

webpack locked as too heated and limited conversation to collaborators Apr 17, 2019

Use location.port when location.hostname is used to infer HMR socket URL #1664

Use location.port when location.hostname is used to infer HMR socket URL #1664

Conversation

rlamana commented Feb 13, 2019 • edited Loading

Motivation / Use-Case

Breaking Changes

alexander-akait left a comment

Choose a reason for hiding this comment

rlamana commented Feb 15, 2019

alexander-akait commented Feb 15, 2019

rlamana commented Feb 20, 2019 • edited Loading

alexander-akait commented Feb 20, 2019

codecov bot commented Feb 27, 2019 • edited Loading

Codecov Report

rlamana commented Feb 27, 2019 • edited Loading

alexander-akait commented Mar 4, 2019

rlamana commented Mar 4, 2019 • edited Loading

alexander-akait commented Mar 4, 2019

alexander-akait commented Mar 4, 2019

alexander-akait commented Mar 5, 2019

rlamana commented Mar 5, 2019

rlamana commented Mar 5, 2019 • edited Loading

hiroppy commented Mar 5, 2019

rlamana commented Mar 5, 2019

hiroppy left a comment

Choose a reason for hiding this comment

hiroppy commented Mar 6, 2019

alexander-akait Mar 6, 2019

Choose a reason for hiding this comment

rlamana Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

rlamana Mar 7, 2019

Choose a reason for hiding this comment

alexander-akait left a comment

Choose a reason for hiding this comment

pedrofurtado commented Apr 8, 2019 • edited Loading

pedrofurtado commented Apr 8, 2019

nick commented Apr 8, 2019

rlamana commented Apr 8, 2019

rlamana commented Apr 8, 2019

jcudzich commented Apr 9, 2019

chenyong commented Apr 9, 2019 • edited Loading

pedrofurtado commented Apr 9, 2019 • edited Loading

alexander-akait commented Apr 9, 2019

pedrofurtado commented Apr 9, 2019

alexander-akait commented Apr 9, 2019

pedrofurtado commented Apr 9, 2019

pedrofurtado commented Apr 10, 2019 • edited Loading

hiroppy commented Apr 10, 2019

leoliu commented Apr 12, 2019

alexander-akait commented Apr 12, 2019

timboslice69 commented Apr 16, 2019

alexander-akait commented Apr 16, 2019

gtzampanakis commented Apr 17, 2019

alexander-akait commented Apr 17, 2019

rlamana commented Feb 13, 2019 •

edited

Loading

rlamana commented Feb 20, 2019 •

edited

Loading

codecov bot commented Feb 27, 2019 •

edited

Loading

rlamana commented Feb 27, 2019 •

edited

Loading

rlamana commented Mar 4, 2019 •

edited

Loading

rlamana commented Mar 5, 2019 •

edited

Loading

rlamana Mar 7, 2019 •

edited

Loading

pedrofurtado commented Apr 8, 2019 •

edited

Loading

chenyong commented Apr 9, 2019 •

edited

Loading

pedrofurtado commented Apr 9, 2019 •

edited

Loading

pedrofurtado commented Apr 10, 2019 •

edited

Loading