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: require serialization for HMRConnection.send on implementation side #18186

Merged

Conversation

hi-ogawa
Copy link
Collaborator

@hi-ogawa hi-ogawa commented Sep 24, 2024

Description

This is brought up by @jamesopstad (Also thanks for spotting the bug on my HMRConnection implementation!).

Currently HMRConnection.send and HMRConnection.onUpdate interfaces are unbalanced as HMRConnection.send: (message: string) => void receives already serialized string of HotPayload. Custom webSocket based implementation would require something like this:
https://github.com/hi-ogawa/vite-environment-examples/blob/edf04cf23fb472a9d085e1a424ae7543164b98f8/packages/workerd/src/worker.ts#L95-L107

      hmr: {
        connection: {
          isReady: () => true,
          onUpdate(callback: (payload: HotPayload) => void) {
            webSocket.addEventListener("message", (event) => {
              callback(JSON.parse(event.data));
            });
          },
          send(messages: string) {
            webSocket.send(messages);
          },
        },
      },

This PR changes it to HMRConnection.send: (payload: HotPayload) => void, so HMRConnection implementation will become like below:

      hmr: {
        connection: {
          ...
          send(payload: HotPayload) {
            webSocket.send(JSON.stringify(payload));
          },
        },
      },

I think this will make interfaces more uniform since HotChannel on server side also assumes (de)serialization to be taken care by implementations. Also in case of ServerHMRConnector / ServerHotChannel, this removes unnecessary serialization since they can pass payload object directly.

Copy link

stackblitz bot commented Sep 24, 2024

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

sheremet-va
sheremet-va previously approved these changes Sep 24, 2024
@sheremet-va
Copy link
Member

Looks good to me! Funny how we missed it 😄

@hi-ogawa hi-ogawa marked this pull request as ready for review September 24, 2024 09:14
*/
isReady(): boolean
/**
* Send a message to the client.
* Send a message to the server.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also I remembered this one was saying the opposite, so updated it as well.

@hi-ogawa hi-ogawa changed the title refactor: require serialization for HMRConnection.send implementation fix!: require serialization for HMRConnection.send implementation Sep 24, 2024
@hi-ogawa hi-ogawa changed the title fix!: require serialization for HMRConnection.send implementation fix!: require serialization for HMRConnection.send on implementation side Sep 24, 2024
Copy link

@jamesopstad jamesopstad left a comment

Choose a reason for hiding this comment

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

Thanks for making these changes. Looks great to me!

@patak-dev patak-dev changed the title fix!: require serialization for HMRConnection.send on implementation side fix: require serialization for HMRConnection.send on implementation side Sep 26, 2024
@patak-dev patak-dev merged commit 9470011 into vitejs:main Sep 26, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants