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: infer hmr ws target by client location #8650

Merged

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Jun 18, 2022

Description

This PR implements #7977 (comment).
(That comment says "If we default hmr to same port with server" but it was already working like that.)

I created https://github.com/sapphi-red/vite-setup-catalogue and tested all of these setups and it worked without any configuration.
(with-proxy includes a configuration because this repo uses 3.0.0-alpha.12 not this PR)

fixes #3093
fixes #5153
fixes #6814

Cases covered with vite-setup-catalogue

Cases not covered with vite-setup-catalogue

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@sapphi-red sapphi-red added feat: hmr p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) labels Jun 18, 2022
@patak-dev
Copy link
Member

This is amazing @sapphi-red: https://github.com/sapphi-red/vite-setup-catalogue
I wonder if we should add all these cases to our CI or maybe add some tests in your repo and connect it to vite-ecosystem-ci.
@lukashass @lbogdan if you have some time, please review and let us know about this approach 🙏🏼
Also @davidwallacejackson, in case you have time to retest with your setup

@sapphi-red
Copy link
Member Author

sapphi-red commented Jun 18, 2022

The with-proxy setup in vite-setup-catalogue requires docker so I think it is better to use vite-ecosystem-ci. (or remove docker)

I need to solve how to make websocket proxy by-passed setup work without configuration.

@patak-dev
Copy link
Member

I think vite-ecosystem-ci would be a good place for this 👍🏼
It will only require adding a test file like https://github.com/vitejs/vite-ecosystem-ci/blob/main/tests/marko.ts

@sapphi-red
Copy link
Member Author

I need to solve how to make websocket proxy by-passed setup work without configuration.

I implemented the fallback feature which I said in the description of this PR: "To avoid a breaking change: If the connection failed, I think we could fallback to the Vite HMR server."

docs/config/server-options.md Outdated Show resolved Hide resolved
docs/config/server-options.md Outdated Show resolved Hide resolved
@patak-dev
Copy link
Member

@lukashass @lbogdan we're merging this PR because it looks like it covers the required cases, and it has been tested in several scenarios thanks to @sapphi-red's new suite. We would like to have this change in the v3 beta (to be released soon) to get proper feedback from the ecosystem. We still have time to modify the default behavior if you find issues with this approach. Please let us know if this works for your use cases.

@patak-dev patak-dev merged commit 4061ee0 into vitejs:main Jun 20, 2022
@patak-dev
Copy link
Member

Released as vite@3.0.0-alpha.14 in case, so it should be easier to test 👍🏼

@sapphi-red sapphi-red deleted the fix/infer-ws-target-by-client-location branch June 21, 2022 12:22
const socketHost = `${__HMR_HOSTNAME__ || location.hostname}:${__HMR_PORT__}`
const hmrPort = __HMR_PORT__
const socketHost = `${__HMR_HOSTNAME__ || importMetaUrl.hostname}:${
hmrPort || importMetaUrl.port
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self: I forgot to handle a case when importMetaUrl.port is ''. But surprisingly this actually works.

new URL("ws://localhost:").port // ''
new WebSocket("ws://localhost:") // connects to `ws://localhost`

@naruaway
Copy link
Contributor

I think this fix does not work with provided HMR WS server, which is important case to achieve "single port for everything".
I tried to fix it in #11487

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: hmr p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
4 participants