Skip to content

fix(miniflare): support WebSocket proxying over dev registry#10273

Merged
edmundhung merged 1 commit intomainfrom
edmundhung/dev-registy-web-socket-proxy
Aug 8, 2025
Merged

fix(miniflare): support WebSocket proxying over dev registry#10273
edmundhung merged 1 commit intomainfrom
edmundhung/dev-registy-web-socket-proxy

Conversation

@edmundhung
Copy link
Member

Fixes n/a.


  • Tests
    • Tests included
    • Tests not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: bugfix
  • Wrangler V3 Backport
    • Wrangler PR:
    • Not necessary because: miniflare dev registry is v4 only

@changeset-bot
Copy link

changeset-bot bot commented Aug 7, 2025

🦋 Changeset detected

Latest commit: 9b0d4ec

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
miniflare Patch
@cloudflare/pages-shared Patch
@cloudflare/vite-plugin Patch
@cloudflare/vitest-pool-workers Patch
wrangler Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pkg-pr-new
Copy link

pkg-pr-new bot commented Aug 7, 2025

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@10273

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@10273

miniflare

npm i https://pkg.pr.new/miniflare@10273

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@10273

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@10273

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@10273

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@10273

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@10273

wrangler

npm i https://pkg.pr.new/wrangler@10273

commit: 9b0d4ec

@edmundhung edmundhung force-pushed the edmundhung/dev-registy-web-socket-proxy branch from 413edbd to 8897ef2 Compare August 7, 2025 18:43
@edmundhung edmundhung marked this pull request as ready for review August 8, 2025 10:09
@edmundhung edmundhung requested a review from a team as a code owner August 8, 2025 10:09
@edmundhung edmundhung force-pushed the edmundhung/dev-registy-web-socket-proxy branch from 8897ef2 to 9b0d4ec Compare August 8, 2025 10:20
@edmundhung edmundhung changed the title fix(miniflare): support WebSocket proxying to workerd fix(miniflare): support WebSocket proxying over dev registry Aug 8, 2025
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Nice!

const statusLine = `HTTP/1.1 ${upRes.statusCode ?? 101} ${upRes.statusMessage ?? "Switching Protocols"}\r\n`;
let headersString = "";
for (let i = 0; i < upRes.rawHeaders.length; i += 2) {
headersString += `${upRes.rawHeaders[i]}: ${upRes.rawHeaders[i + 1]}\r\n`;
Copy link
Contributor

Choose a reason for hiding this comment

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

OOC is the \r\n rather than just \n to ensure it is valid on Windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just because the HTTP/1.1 Spec requires these lines to be terminated with CLRF (\r\n): https://datatracker.ietf.org/doc/html/rfc7230#section-3

wsResponse.webSocket.accept();

// Test bidirectional communication
wsResponse.webSocket.send("ping");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a chance that this ping will cause a response before you have setup the listener?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. I will move this after the messagePromise in a follow up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 6b3e487 from #10281

@github-project-automation github-project-automation bot moved this from Untriaged to Approved in workers-sdk Aug 8, 2025
@edmundhung edmundhung merged commit 1479fd0 into main Aug 8, 2025
57 of 63 checks passed
@edmundhung edmundhung deleted the edmundhung/dev-registy-web-socket-proxy branch August 8, 2025 13:54
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk Aug 8, 2025
edmundhung added a commit that referenced this pull request Aug 8, 2025
edmundhung added a commit that referenced this pull request Aug 11, 2025
…v registry (#10281)

* fix(miniflare): enable HTTPS support when proxying to workerd

* followup fix from #10273
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants