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: ws never connects after restarting server if server.hmr.server is set #14127

Merged

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Aug 16, 2023

Description

ws never connected after restarting the server if server.hmr.server is set (e.g. Storybook).

/cc @IanVS

Reproduction

  1. Open https://stackblitz.com/github/storybookjs/sandboxes/tree/next/react-vite/default-js/after-storybook?preset=node
  2. Open the devtools and select the Network tab (without filter)
  3. Wait for storybook starts completely
  4. Add .env file in the project root directory
  5. See many polling request + failing WS request happening
  6. Close the tab quickly (otherwise the tab will crash 😬)

How it happens

If server.hmr.server is not set, when the Vite server restarts, this wsServer becomes a different instance.

const wsServer = hmrServer || (portsAreCompatible && server)

But since Storybook passes their server to server.hmr.server, even if Vite's server restarted, wsServer is a same instance. Then, the old listener swallows all WebSocket connections and the new listener (started listening after the server restarted) can't handle any requests.

Additional context

Found this bug while looking around storybookjs/storybook#22253.


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 PR Title 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 the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Aug 16, 2023
@stackblitz
Copy link

stackblitz bot commented Aug 16, 2023

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

@IanVS
Copy link
Contributor

IanVS commented Aug 16, 2023

Oh wow, 2 hours from report to a PR with a bugfix. Thanks so much!

image

The other part of the Storybook issue that confuses me, is why vite is detecting changes to .env and/or tsconfig.json in the first place. This seems like a reasonable fix to the problem of ws not reconnecting, but do you have any theories on why it happens in the first place?

@sapphi-red
Copy link
Member Author

sapphi-red commented Aug 16, 2023

I don't know why #14126 happens. It didn't happen on my machine (#14043 (comment)). I was trying to reproduce the same situation by running a script that changes the .env file in a infinite while loop, and found this bug 😅.

@IanVS
Copy link
Contributor

IanVS commented Aug 16, 2023

I find that it's pretty intermittent. I tend to notice it happening more when changing branches, or installing/removing dependencies. I would have opened an issue a long time ago if I could create a reproduction, but so far haven't been able to. :(

@patak-dev patak-dev merged commit bd9b749 into vitejs:main Aug 16, 2023
11 checks passed
@TNAJanssen
Copy link

It feels good (sorry for you guys) that I'm not the only one and not crazy 😆. That said, it even happened to me once on a completely new project, and after restarting the server it went away. For my local machine with my main project, I can't even start the server anymore without ignoring all the file that trigger a restart.

@sapphi-red sapphi-red deleted the fix/restart-server-ws-never-connects branch August 17, 2023 12:22
@IanVS
Copy link
Contributor

IanVS commented Aug 21, 2023

This seems to be caused by a problem in fsevents: paulmillr/chokidar#1286. It has been fixed, but not yet released there. Patching one line has been reported to prevent the issue for several Storybook users.

@bluwy bluwy mentioned this pull request Oct 3, 2023
sapphi-red added a commit to sapphi-red/vite-setup-catalogue that referenced this pull request Aug 15, 2024
sapphi-red added a commit to sapphi-red/vite-setup-catalogue that referenced this pull request Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants