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

feat: show ws connection error and don't reload if ws connection did not success #9007

Merged
merged 1 commit into from
Jul 9, 2022

Conversation

sapphi-red
Copy link
Member

Description

#8650 introduced a fallback mechanism when the connection did not success.
This PR uses that to prevent reload if ws connection did not success.
Also this PR adds an error output when that happened.

image

fixes #8861 (because this PR will prevent reload if ws did not connect)
fixes #6089, close #6090 (because this PR will prevent reload if ws did not connect)

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 the p2-nice-to-have Not breaking anything but nice to have (priority) label Jul 9, 2022
@netlify
Copy link

netlify bot commented Jul 9, 2022

Deploy Preview for vite-docs-main canceled.

Name Link
🔨 Latest commit 07543af
🔍 Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62c973f725f7b100081c3e77

@patak-dev
Copy link
Member

Would the auto-reconnect feature of Vite still work after this PR? (I think this isn't an accidental feature, no?)

Scenario:

  • Vite client connects properly
  • The server at one point is closed
  • Vite client pings and fails
  • Vite keeps pinging...
  • The server is up again
  • Next ping is successful and the page is reloaded

@sapphi-red
Copy link
Member Author

Yes, it still works with that scenario.

Once websocket's readyState changes to OPEN, fallback function won't be called

const socket = new WebSocket(`${protocol}://${hostAndPath}`, 'vite-hmr')
let isOpened = false
socket.addEventListener(
'open',
() => {
isOpened = true
},
{ once: true }
)
// Listen for messages
socket.addEventListener('message', async ({ data }) => {
handleMessage(JSON.parse(data))
})
// ping server
socket.addEventListener('close', async ({ wasClean }) => {
if (wasClean) return
if (!isOpened && onCloseWithoutOpen) {
onCloseWithoutOpen()
return
}

  • open event sets isOpened = true
  • fallback function (param name is onCloseWithoutOpen) is called only when close event happened and isOpened is false

@sapphi-red
Copy link
Member Author

sapphi-red commented Jul 9, 2022

One scenario that will no longer work is:

  • Vite server starts but websocket server does not yet
  • browser shows the page
  • Vite client fails to connect websocket (since websocket server is not started yet)
  • websocket server starts
  • pinging/reload won't happen (pinging/reload was happening before)

@patak-dev
Copy link
Member

Should we retry a few times so that scenario keeps working?

@sapphi-red
Copy link
Member Author

I don't think we need to support this scenario. I didn't come up with an actual situation that this scenario will happen.

Vite starts server and ws server at the same time. So when /@vite/client is accessible by browser, ws is possible to connect.
This is same with middleware mode. To register the middleware, createServer needs to be called, and ws server is started inside that.

@niksy
Copy link
Contributor

niksy commented Dec 9, 2022

@sapphi-red

One scenario that will no longer work is:

  • Vite server starts but websocket server does not yet
  • browser shows the page
  • Vite client fails to connect websocket (since websocket server is not started yet)
  • websocket server starts
  • pinging/reload won't happen (pinging/reload was happening before)

Is this the scenario where someone is using Nodemon to restart whole server? Because I have situation where Nodemon performs restarts, Vite loses WebSocket connection, and if it doesn’t find it again (e.g. Nodemon hasn’t started new application fast enough), page gets reloaded.

It would be nice to somehow either prevent reload, or try to ping for WebSocket connection few more times to prevent page reload.

@sapphi-red
Copy link
Member Author

@niksy No, that's different from what I said.

For that case, I think implementing #5675 would work.

@niksy
Copy link
Contributor

niksy commented Dec 11, 2022

@sapphi-red any pointers on what needs to be done to implement this?

@sapphi-red
Copy link
Member Author

@niksy I'll leave a comment on #5675 👍

@zanetarock
Copy link

@sapphi-red Hello, could there possibly be a reason why this fallback works for Chrome but not on Firefox/Safari? I deployed a vite server but for some reasons websocket doesn't work. Then on chrome I can see the error messages, but for Firefox/Safari the page keeps reloading.

@sapphi-red
Copy link
Member Author

@zanetarock Without a reproduction, I don't know. Please create an issue.

@aparajita
Copy link

@sapphi-red Unfortunately this is not working with WebKit-based hybrid apps.

@patak-dev
Copy link
Member

@aparajita please create a new bug report so we can properly track the issue, you can link to this PR in it. A comment on an old merged PR would probably end up lost quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
5 participants