fix(client): wait on the socket host, not the ping host

[nicks]

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]

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

[datio]

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]

Would you check if #5466 solves your issue?

[nicks]

@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}
  }
})

3. Run yarn dev and visit the page.

[patak-dev]

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]

@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]

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]

@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]

@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]

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.

[nicks]

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