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(client): wait on the socket host, not the ping host #6819

Merged
merged 1 commit into from
May 10, 2022

Conversation

nicks
Copy link
Contributor

@nicks nicks commented Feb 9, 2022

fix(client): if the websocket fails to connect, wait on the socket host, not the ping host

The problem with checking the ping host is that the ping host can
pass, even if the socket host fails, leading to infinite reloads.

Infinite reloads are a bad way to deal with this failure.
This makes the failure less bad.

For examples, see:
#6814
#3093

Description

There are many bug reports where Vite infinitely reloads if there's an HMR misconfiguration. This doesn't fix the misconfiguration, but fixes the infinite loop.

Additional context

I tested this PR manually. I looked around for a way to exercise this in a unit test or integration test, but couldn't figure out a good way to do it. I'm open to ideas.


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.

@nicks
Copy link
Contributor Author

nicks commented Feb 9, 2022

related: #6090 (which fixes this with a configuration option)

@bluwy bluwy added the p3-minor-bug An edge case that only affects very specific usage (priority) label Feb 9, 2022
@datio
Copy link

datio commented Feb 10, 2022

This stopped the constant reload issue on a reverse proxy issue I have with nginx (docker nginx container reverse proxying my svelte dev app that is running on host)

But the request keeps failing with:
Fetch failed loading: GET "<URL>".

@patak-dev
Copy link
Member

Would you check if #5466 solves your issue?

@nicks
Copy link
Contributor Author

nicks commented Feb 10, 2022

@patak-dev Nope, #5466 fixes a different issue.

But I think the fact that there are multiple PRs open for different bugs in this code points to a larger issue. The high-level approach to reloads here is flawed and prone to infinite loops. I tried to make my change as narrow as possible, but it's likely this will need additional fixes if the vite project sticks to it.

If you want to test it yourself, an easy way to do it is:

  1. Create a sample project yarn create vite
  2. Deliberately misconfigure the HMR with a vite.config.js like:
import { defineConfig } from 'vite'

export default defineConfig({
  server: {
    hmr: {clientPort: 3002}
  }
})
  1. Run yarn dev and visit the page.

@patak-dev
Copy link
Member

What is the bigger change that you have in mind here @nicks? Even if we end up merging some hotfixes to the current approach it is good to start discussing it.

@nicks
Copy link
Contributor Author

nicks commented Feb 10, 2022

@patak-dev

from a behavior standpoint: the page should only refresh in the case where we're certain we got a successful websocket connection then lost it. We should not refresh in the case where the websocket connection is simply broken from the get-go. And we should err on the side of not refreshing.

To implement this, you'd need some kind of persistent storage across page refreshes (e.g., LocalStorage, or cookie, or history param would all work fine):

  • When the page first loads set the token SAFE_TO_RELOAD=0
  • When the websocket connects, set the token SAFE_TO_RELOAD=1
  • When the websocket disconnects, only refresh if the token SAFE_TO_RELOAD==1 and the server is responding to requests.

@ssendev
Copy link

ssendev commented Apr 24, 2022

I hit the same problem and wanted to make a pull request but i had left the ${base}__vite_ping in there so it still hits the same endpoint.

const pingResponse = await fetch(`${location.protocol}//${socketHost}${base}__vite_ping`);

Reading the comments here maybe just trying to connect to the websocket again is less error prone since it's guaranteed to be the correct address and doesn't suffer from the issue with false reloads in case of non functioning/disabled websockets.

here's another issue with the same problem #2345

@patak-dev
Copy link
Member

@jessarcher did you try this PR? You mentioned in the linked issue that no PR was fixing the original issue, but IIUC this would already help, no?

@ssendev I'm thinking the same, we could remove the ping middleware and merge this now that we are going to release Vite v3 beta. So there is time for the ecosystem to react if we hit an issue.

@jessarcher
Copy link
Contributor

@patak-dev Yes, this does fix the issue :) Sorry for the confusion - in my comment, I was referring to the config-based fixes that were suggested.

@patak-dev
Copy link
Member

Hey @nicks, would you merge main to resolve the conflicts? We are going to move forward with your scheme in v3 and test it during the beta.

This is conflicting with #5466, but the changes it did can be removed. @mwarnerdotme, if possible, please test that this PR works for your setup correctly. Thanks.

…st, not the ping host

The problem with checking the ping host is that the ping host can
pass, even if the socket host fails, leading to infinite reloads.

Infinite reloads are a bad way to deal with this failure.
This makes the failure less bad.

For examples, see:
vitejs#6814
vitejs#3093
@nicks
Copy link
Contributor Author

nicks commented May 10, 2022

OK, rebased! And ya, I reverted #5466 because its fix was incompatible with this fix.

@patak-dev patak-dev changed the title fix(client): if the websocket fails to connect, wait on the socket host, not the ping host fix(client): wait on the socket host, not the ping host May 10, 2022
@patak-dev patak-dev merged commit ae56e47 into vitejs:main May 10, 2022
wangjianio added a commit to wangjianio/vite that referenced this pull request May 12, 2022
vitejs#6819 changed the way to reconnect hmr, `/__vite_ping` is useless now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants