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

Regression: web socket connection fails with @webpack-cli/serve@1.4.0 when using findPort #3351

Closed
joeldenning opened this issue May 7, 2021 · 8 comments · Fixed by #3467

Comments

@joeldenning
Copy link
Contributor

Describe the bug

When the port is left unspecified (both in CLI args and in webpack config), webpack-dev-server finds a port, defaulting to 8080 when possible. However, the webpack-dev-server client code is not aware of which port was chosen, and the resource query for setting up the web socket is unaware of that port. This causes the webpack-dev-server client code to attempt to connect to the wrong port when a bundle is loaded on a different port from the host html file

This bug was initially reported in #2873, and also in #2142 for older versions of webpack. I fixed it both times, in webpack/webpack-cli#2147 and #2143

@webpack-cli/serve@1.4.0 appears to have caused a regression, where the bug is back. I think it is because of webpack/webpack-cli#2648, although haven't confirmed if it's that PR or not. From the code in webpack/webpack-cli#2648 it seems very likely to be related.

The regression was initially brought to my attention in single-spa/create-single-spa#293 by @PieterBoeren.

What is the current behavior?

When you run webpack-dev-server without a port, it finds a port but the client code doesn't know which port was found and therefore does not attempt a web socket connection to the correct port.

To Reproduce

Steps to reproduce the behavior:

I can create a Github repo for this if needed. I created https://github.com/joeldenning/wds-port-bug for an older version of webpack-cli and can create a new one for this newer version, if necessary

Expected behavior

The port found by findPort should be passed to the client code as a __resourceQuery. The client should connect to the server on the correct port

Screenshots

(should be connecting to port 8080, but is not)

image

Please paste the results of webpack-cli info here, and mention other relevant information

pnpx webpack-cli info

  System:
    OS: macOS 11.2.3
    CPU: (12) x64 Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
    Memory: 789.29 MB / 16.00 GB
  Binaries:
    Node: 14.16.0 - /usr/local/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 6.14.11 - /usr/local/bin/npm
  Browsers:
    Chrome: 90.0.4430.93
    Firefox: 85.0
    Safari: 14.0.3
  Packages:
    webpack: ^5.8.0 => 5.36.2 
    webpack-cli: ^4.2.0 => 4.7.0 
    webpack-config-single-spa-react: ^2.0.0 => 2.2.2 
    webpack-dev-server: ^4.0.0-beta.0 => 4.0.0-beta.3 
    webpack-merge: ^5.4.0 => 5.7.3

Additional context

@joeldenning joeldenning changed the title Regression: web socket connection fails after 1.4.0 when using findPort Regression: web socket connection fails with @webpack-cli/serve@1.4.0 when using findPort May 7, 2021
@anshumanv
Copy link
Member

Was reading dev-server code and it seems we're setting a port in server -

this.options.port = port;

And calc for port is taking into account the port we found -

But it's not being passed here, we're just passing default options -

__webpack_dev_server_client__: getSocketClientPath(options),

I think setting options.client.port in the server should fix this? Happy to PR if this is the right direction

/cc @alexander-akait

@anshumanv
Copy link
Member

Actually, I'm unable to repro this in a blank config in your old repo with updated packages @joeldenning, could you possibly create a branch in the repo for this bug? Thanks!

@joeldenning
Copy link
Contributor Author

Thanks @anshumanv for looking into this. I've pushed to https://github.com/joeldenning/wds-port-bug/tree/webpack-cli-issue-2696 (the webpack-cli-issue-2696 branch) and have confirmed that the issue persists. The instructions in the Readme still apply for repro.

@alexander-akait
Copy link
Member

alexander-akait commented May 13, 2021

oh, we endlessly rush between attempts to solve problems with proxied server and non proxied, after fixing bug for non proxied we get a new bug for proxied and versa vice. Honestly if I write this server from scratch I say: always set client.port when you proxy port (same for host).

The problem - we trying to convert dev server to plugin, so we need inject entry with options before listen, but we don't know port before listen...

Also here were other problems between host/port/path of websocket server and client options to websocket client.

No ideas how to solve them together. It will blow my head soon.

@alexander-akait
Copy link
Member

Let's keep open before stable release, maybe I found way... if not I will document it and mask as breaking change.

For me using client.host/client.port/client.path when you proxy is most good solution, we don't need complex logic for client.

@alexander-akait
Copy link
Member

Also it will allow us to remove the public option (I found it very dirty and unclean) in favor client.host/client.port/client.path. And also you can do any complex proxy.

@joeldenning
Copy link
Contributor Author

joeldenning commented May 13, 2021

👍🏿 that option seems sensible to me - thanks for investigating it

@alexander-akait
Copy link
Member

Found the way how we will fix it without breaking a plugin support, I will try to fix this in near future to the next 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 a pull request may close this issue.

3 participants