Skip to content

Comments

fix(server): recover server.ws.clients.send API changes for back compatibility#18779

Closed
antfu wants to merge 2 commits intomainfrom
fix/server-ws-client-send-api
Closed

fix(server): recover server.ws.clients.send API changes for back compatibility#18779
antfu wants to merge 2 commits intomainfrom
fix/server-ws-client-send-api

Conversation

@antfu
Copy link
Member

@antfu antfu commented Nov 26, 2024 •
edited
Loading

During refactoring of Environment API, I think we mistakenly change the send signature on WebSocketClient, causing a breaking regression:

Vite 5:

vite/packages/vite/src/node/server/ws.ts

Lines 65 to 72 in 81e6c04

/**
* Send event to the client
*/
send(payload: HMRPayload): void
/**
* Send custom event
*/
send(event: string, payload?: CustomPayload['data']): void

Vite v6.0.0 (WebSocketClient extends HotChannelClient):

vite/packages/vite/src/node/server/hmr.ts

Lines 78 to 80 in c7b3308

export interface HotChannelClient {
send(payload: HotPayload): void
}

@antfu antfu requested a review from patak-dev November 26, 2024 14:00
@sheremet-va
Copy link
Member

sheremet-va commented Nov 26, 2024

I am not sure this was done by mistake, the payload in channels should now be always normalized 🤔

@sheremet-va
Copy link
Member

sheremet-va commented Nov 26, 2024

The PR that changes how hot channels work: #18362

I think one of the goals was to make it easier to work from the outside to remove the if - maybe it is not wrapped by mistake?

@antfu
Copy link
Member Author

antfu commented Nov 26, 2024

Ah I see. I get the clients by for (const client of server.ws.clients) which seems to be a public API.

@sapphi-red
Copy link
Member

sapphi-red commented Nov 26, 2024

I didn't expect to break things on server.ws and I think we can keep this compat. I pushed a commit that fixes the type (The Client should not extend HotChannel).

@sapphi-red sapphi-red added p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) regression The issue only appears after a new release labels Nov 26, 2024
@antfu
Copy link
Member Author

antfu commented Nov 26, 2024

Thanks! Indeed, that makes more sense! This PR doesn't seems to fully solve my case on vite-plugin-inspect yet, I will keep investigating and send other PRs if any

@antfu
Copy link
Member Author

antfu commented Nov 26, 2024 •
edited
Loading

After some digging, I found the problem is that in that in vite-dev-rpc:

https://github.com/antfu/vite-dev-rpc/blob/b6899623ed19297e5927262396f0d4bfdc34016b/src/index.ts#L18-L28

L27 uses server.ws.clients.send with two args API, which this PR fixes.

While L22 compared the matched source with the new branch, the hot client always gets normalized; thus, the instances are different. And during the normalization, source.socket gets lost as undefined

vite/packages/vite/src/node/server/hmr.ts

Lines 233 to 254 in e26acf4

const listenerWithNormalizedClient = (
data: any,
client: HotChannelClient,
) => {
const normalizedClient: NormalizedHotChannelClient = {
send: (...args) => {
let payload: HotPayload
if (typeof args[0] === 'string') {
payload = {
type: 'custom',
event: args[0],
data: args[1],
}
} else {
payload = args[0]
}
client.send(payload)
},
}
fn(data, normalizedClient)
}
normalizedListenerMap.set(fn, listenerWithNormalizedClient)

I will come up with an alternative PR to propose the fix

@antfu antfu mentioned this pull request Nov 26, 2024
Closed
@antfu
Copy link
Member Author

antfu commented Nov 26, 2024

Raised #18781

@antfu antfu mentioned this pull request Nov 26, 2024
Merged
@patak-dev
Copy link
Member

patak-dev commented Nov 27, 2024

Closing as #18782 has been merged

@patak-dev patak-dev closed this Nov 27, 2024
@sapphi-red sapphi-red deleted the fix/server-ws-client-send-api branch April 3, 2025 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sapphi-red sapphi-red sapphi-red approved these changes

@patak-dev patak-dev Awaiting requested review from patak-dev

Assignees

No one assigned

Labels

p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) regression The issue only appears after a new release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@antfu @sheremet-va @sapphi-red @patak-dev