-
Notifications
You must be signed in to change notification settings - Fork 9.9k
Update WebSocket examples #18535
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
Update WebSocket examples #18535
Conversation
Deploying cloudflare-docs with
|
| Latest commit: |
f2b809e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://41100605.cloudflare-docs-7ou.pages.dev |
| Branch Preview URL: | https://do-websocket-examples.cloudflare-docs-7ou.pages.dev |
|
Closes #24141 |
|
This pull request requires reviews from CODEOWNERS as it changes files that match the following patterns:
|
|
Preview URL: https://a1f4e741.preview.developers.cloudflare.com Files with changes (up to 15)
|
| let attachment = ws.deserializeAttachment(); | ||
| if (ws.deserializeAttachment()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong?
| const { ...session } = attachment; | ||
| this.sessions.set(ws, { ...session }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the spreading and then recollecting? Just pass attachment or if you only want its properties spread that directly.
| const { ...session } = attachment; | |
| this.sessions.set(ws, { ...session }); | |
| this.sessions.set(ws, { ...attachment }); |
| this.sessions.forEach((attachment, session) => { | ||
| session.send(`[Durable Object] message: ${message}, from: ${attachment.id}. Total connections: ${this.sessions.size}`); | ||
| }); | ||
|
|
||
| // Send a message to all WebSocket connections except the connection (ws), | ||
| // loop over all the connected WebSockets and filter out the connection (ws). | ||
| this.sessions.forEach((attachment, session) => { | ||
| if (session !== ws) { | ||
| session.send(`[Durable Object] message: ${message}, from: ${attachment.id}. Total connections: ${this.sessions.size}`); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combine these two loops?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loops are kept separate to make it easy for users to simply copy-paste based on their use-case.
| async fetch(request: Request, env: Env, ctx: ExecutionContext): Promise<Response> { | ||
| if (request.url.endsWith('/websocket')) { | ||
| // Expect to receive a WebSocket Upgrade request. | ||
| // If there is one, accept the request and return a WebSocket Response. | ||
| const upgradeHeader = request.headers.get('Upgrade'); | ||
| if (!upgradeHeader || upgradeHeader !== 'websocket') { | ||
| return new Response('Worker expected Upgrade: websocket', { | ||
| status: 426, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting indentation is messed up here, and below.
| // Broadcast the message to all the connections, | ||
| // except the one that sent the message. | ||
| this.sessions.forEach((k, session) => { | ||
| if (session !== ws) { | ||
| session.send(`[Durable Object] message: ${message}, from: ${connection.id}`); | ||
| } | ||
| }); | ||
|
|
||
| // Broadcast the message to all the connections, | ||
| // including the one that sent the message. | ||
| this.sessions.forEach((k, session) => { | ||
| session.send(`[Durable Object] message: ${message}, from: ${connection.id}`); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge the loops.
|
|
||
| return stub.fetch(request); | ||
| } | ||
| return stub.fetch(request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than removing the extra stub.fetch, what's the diff here?
| description: Build a WebSocket server using WebSocket Hibernation on Durable | ||
| Objects and Workers. | ||
| --- | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix this. It seems to worsen a subsequent diff by a lot.
joshthoward
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harshil1712 git rebase -i production is your friend. There are a lot of changes here which are unsubstantiated, e.g. large changes which only adjust indentation, and inconsistent, e.g. changing ```` to ``` in some cases. Please clean up this PR so that it can be reviewed properly.
Oh sorry @harshil1712... @Oxyjun contributed the later commits. The advice still stands... Please clean up this PR so that it can be reviewed properly. |
|
We're now tracking this here: #24234 |
Summary
Updates the WebSocket and WebSocket Hibernation examples.