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(client): add WebSocket connections events #13334

Merged
merged 2 commits into from Jun 15, 2023
Merged

Conversation

antfu
Copy link
Member

@antfu antfu commented May 24, 2023

Description

Currently, when Vite's WS connection close/reconnects, it only prints to the console, but no way for other integrations to know it programmatically.

This PR introduced two client events, vite:ws:connect and vite:ws:disconnect. This would allow plugins and meta frameworks to display better connection indicators within UI, improving the DX by some extent.

Additional context

Motivation on my side is: nuxt/devtools#243


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented May 24, 2023

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

@antfu antfu added feat: hmr p2-nice-to-have Not breaking anything but nice to have (priority) labels May 24, 2023
patak-dev
patak-dev previously approved these changes May 24, 2023
@patak-dev patak-dev added this to the 4.4 milestone May 24, 2023
bluwy
bluwy previously approved these changes May 24, 2023
Comment on lines 15 to 16
'vite:ws:connect': WebSocket
'vite:ws:disconnect': WebSocket
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the WebSocket instance? It concerns me a bit that WebSocket instance has a broad interface. (e.g. socket.send())

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't required for the original request. @antfu maybe we should add this param once we have a real use case? And once we go there, it could be a Payload object like the others so we could extend it in the future if needed.

Copy link
Member Author

@antfu antfu Jun 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion either on having it or not. I was just thinking why not, since we have that information.

A few notes added to the consideration:

  • WebSocket it's a web standard so I don't need to worry about the API been changed
  • If we say web socket is an implementation detail, vite:ws already indicates it's web socket. Later if we really find an alternative way of making the connection, those hooks won't be called so there will be no conflicts like the general vite:connect
  • On the node side, plugins already have logics with WebSocket. It's not really an implementation detail we can't reveal.

I don't expect this will be changed and won't add much maintenance interface to us.

I do see one downside is that ppl might start using it and mess it around to break Vite - and then send requests to support very niche usages. But that could happen everywhere and we need to prevent that for sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points, I think we can talk about this in the next team meeting 👍🏼
I assume you also don't think it is a good idea to have a Payload object here.

@antfu antfu dismissed stale reviews from bluwy and patak-dev via a356e4d June 15, 2023 07:25
@patak-dev patak-dev merged commit eb75103 into main Jun 15, 2023
20 checks passed
@patak-dev patak-dev deleted the feat/client-event-ws branch June 15, 2023 12:37
@patak-dev
Copy link
Member

We started a discussion to gather feedback so we can stabilize webSocket in the payload for this feature:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: hmr p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants