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: use websocket to test server liveness before client reload #17891

Merged
merged 19 commits into from
Oct 25, 2024

Conversation

hi-ogawa
Copy link
Collaborator

@hi-ogawa hi-ogawa commented Aug 16, 2024

Description

This PR makes waitForSuccessfulPing more robust by testing Vite server liveness based on websocket connection as suggested in #5675 (comment)

Testing #13125

  • run playground e.g. pnpm -C playground/tailwind dev
  • run proxy e.g. cloudflared tunnel --url http://localhost:5173/
  • open a page from proxy e.g. https://arrow-lonely-looks-counsel.trycloudflare.com/
  • stop playground server and see client keeps pinging (previously client full reloads here and ends up 502 page)
  • start playground server and see client reloads automatically

Testing #16536 (this one is added as a test case)

  • run playground pnpm -C playground/tailwind dev with a following server config
      server: {
        // different port for hmr websocket
        hmr: {
          port: 12345
        },
        // cross origin isolation headers
        headers: {
          "Cross-Origin-Embedder-Policy": "require-corp",
          "Cross-Origin-Opener-Policy": "same-origin"
        }
  • stop playground server and see client keeps pinging
  • start playground server and see client reloads (previously client fails to automatically full reload here)

Copy link

stackblitz bot commented Aug 16, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@hi-ogawa hi-ogawa marked this pull request as ready for review August 17, 2024 08:11
@hendrikheil
Copy link

My team applied these changes to vite locally and tried it with 5 different Nuxt 3 based applications running behind a traefik based https reverse proxy. This change massively improves the DX for us as we no longer have to constantly full-page reload our apps especially when vite is still booting.

I'd love to see this merged ❤️

Thanks @hi-ogawa, this gives us back so much productivity!

@francislavoie
Copy link

francislavoie commented Sep 20, 2024

Can confirm, this also fixes my issue. I use vite for SSR by adding it to my express server (pretty much exactly this code in app.use('*': https://vitejs.dev/guide/ssr#setting-up-the-dev-server), but at the end I add these CORS headers to my SSR rendered response to get access to performance.now()

        res.status(200).set({
          'Content-Type': 'text/html',
          'Cross-Origin-Opener-Policy': 'same-origin',
          'Cross-Origin-Embedder-Policy': 'require-corp',
        }).end(html);

Before this patch, after a server stop/start or reload, the HMR ping over HTTP would loop forever and fail with a 426 status (what Firefox shows me anyway) and console logs show the request failed due to CORS. I couldn't find any easy way to configure the HMR server to add CORS headers after a solid hour of searching.

Eventually landed here, tried this fix, and it works. Pinging over WS instead of over HTTP certainly makes more sense IMO especially since the target connection is WS anyway; why ping over HTTP when the goal is to get a WS connection set up? Browsers have different rules for HTTP vs WS regarding CORS so pinging over WS removes that entire class of issues.

@hi-ogawa
Copy link
Collaborator Author

hi-ogawa commented Sep 27, 2024

New test is flaky but I'm not sure what's going on. Locally, this command can fail if I run a few times.

pnpm test-serve playground/client-reload

@hi-ogawa
Copy link
Collaborator Author

hi-ogawa commented Oct 2, 2024

Though tests could be still flaky, but I added some workaround by skipping some tests and retry.

@sapphi-red sapphi-red added feat: hmr p3-minor-bug An edge case that only affects very specific usage (priority) labels Oct 3, 2024
@sapphi-red
Copy link
Member

It passes on my PC even if I enable all tests. Did you try pnpm debug-serve?

@sapphi-red

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@hi-ogawa
Copy link
Collaborator Author

hi-ogawa commented Oct 3, 2024

It passes on my PC even if I enable all tests. Did you try pnpm debug-serve?

On my PC, it happens quite consistently (like 1 in 2 or 3 runs) by running it while sleep 1; do pnpm test-serve playground/client-reload/; done. Also there a few fails on CI before the commit test: retry.

I couldn't figure out a way to debug the issue. What does debug-serve do?

@sapphi-red
Copy link
Member

What does debug-serve do?

It'll run playwright in non-headless mode. Maybe you can find out what's happening. Although, it may not reproduce with it.

@hi-ogawa
Copy link
Collaborator Author

hi-ogawa commented Oct 3, 2024

Hmm, unfortunately it doesn't reproduce with debug-serve. I think I've only seen this fails on ubuntu CI, so it's possible that this is something due to os + headless specific quirks.

Let me tweak the test to only skip on linux.

@hi-ogawa
Copy link
Collaborator Author

hi-ogawa commented Oct 3, 2024

Well, it just failed on macos as well. I'll re-run a few times and probably I'll put back skip and retry.

This reverts commit 3af732c.
@sapphi-red
Copy link
Member

I fixed the test fail in 91e7edf. It seems the HTTP server was sending an empty response because the WS server listens before the HTTP server.

@sapphi-red

This comment was marked as outdated.

sapphi-red
sapphi-red previously approved these changes Oct 4, 2024
@vite-ecosystem-ci

This comment was marked as outdated.

@sapphi-red
Copy link
Member

Hmm, it seems my change broke a bunch of tests.

@sapphi-red

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@sapphi-red

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@hi-ogawa
Copy link
Collaborator Author

hi-ogawa commented Oct 4, 2024

Wow, awesome finding. I still cannot see it reproduced in the browser, but the fix makes sense!

@sapphi-red
Copy link
Member

I did a unsophisticated push and CI debug😅
https://github.com/sapphi-red/vite/commits/fix/test-fix-robost-client-ping-reolad/

sapphi-red
sapphi-red previously approved these changes Oct 8, 2024
patak-dev
patak-dev previously approved these changes Oct 8, 2024
@patak-dev patak-dev added this to the 6.0 milestone Oct 8, 2024
@patak-dev
Copy link
Member

Great work! I think it would be good to have this in Vite 6. I added it to the milestone.

@sapphi-red sapphi-red dismissed stale reviews from patak-dev and themself via 38942b1 October 25, 2024 04:11
@francislavoie
Copy link

@patak-dev why not in v5.x? This is a great bug fix IMO.

@sapphi-red
Copy link
Member

It because this PR is not small enough or urgent enough to release in a patch and we don't have a plan to release a minor in v5.

@sapphi-red
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 38942b1: Open

suite result latest scheduled
astro failure failure
nuxt failure success
redwoodjs failure failure
remix failure failure
sveltekit failure failure
vike failure failure
vite-plugin-svelte failure failure
vitest failure failure

analogjs, histoire, ladle, laravel, marko, previewjs, quasar, qwik, rakkas, storybook, unocss, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress

@sapphi-red
Copy link
Member

nuxt is failing on main branch and the fails have the same errors with the latest scheduled ones so should be fine.

@sapphi-red sapphi-red merged commit 7f9f8c6 into vitejs:main Oct 25, 2024
14 checks passed
@hi-ogawa hi-ogawa deleted the fix-robust-client-ping-reload branch October 25, 2024 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: hmr p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
5 participants