Skip to content

Conversation

@ghostwriternr
Copy link
Member

@ghostwriternr ghostwriternr commented Oct 30, 2025

Moves the WebSocket connection functionality from a standalone connect() function to a .wsConnect() method on the Durable Object stub.

Before:

await connect(sandbox, request, 8080);

After:

await sandbox.wsConnect(request, 8080);

This provides a more ergonomic API by treating WebSocket connections as a first-class method on the sandbox instance, rather than requiring a separate import and function call. The implementation attaches the method in getSandbox() to avoid RPC serialization issues with WebSocket connections.

@ghostwriternr ghostwriternr self-assigned this Oct 30, 2025
@changeset-bot
Copy link

changeset-bot bot commented Oct 30, 2025

🦋 Changeset detected

Latest commit: 25aac51

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

This PR includes changesets to release 1 package
Name Type
@cloudflare/sandbox 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

@claude
Copy link
Contributor

claude bot commented Oct 30, 2025

Claude Code Review

Summary: This PR successfully moves WebSocket connection functionality from a standalone connect() function to a .wsConnect() method on the DO stub. The implementation is sound and well-tested.

Issues Found

1. Dummy implementation throws on RPC call (sandbox.ts:346-349)

The wsConnect method in the Sandbox class throws "Not implemented here to avoid RPC serialization issues" - but this will fail if anyone calls it directly on the DO instance rather than through the stub. Consider making this more explicit:

wsConnect(request: Request, port: number): Promise<Response> {
  throw new Error('wsConnect must be called on the stub returned by getSandbox(), not directly on the DO instance');
}

2. Type inconsistency in shared/types.ts

The ISandbox interface adds wsConnect at line 739, but the method signature doesn't match the actual implementation. The implementation uses connect(stub) which returns a function, but the interface just declares the method directly. This works but could be clearer in documentation.

3. Generated worker-configuration.d.ts files (16,759 lines)

Two massive auto-generated type files were committed:

  • examples/claude-code/worker-configuration.d.ts (8,383 lines)
  • examples/minimal/worker-configuration.d.ts (8,376 lines)

While the .gitignore was updated to allow these, consider whether these should be in version control or generated at build time. They bloat the PR diff significantly.

Testing Coverage

✅ Excellent test coverage in sandbox.test.ts:

  • WebSocket detection and routing (lines 547-626)
  • wsConnect() method behavior (lines 628-705)
  • Port validation
  • Request property preservation

Architecture Alignment

✅ The change aligns well with the ergonomic API goal. Using sandbox.wsConnect(request, 8080) is cleaner than connect(sandbox, request, 8080).

✅ The getSandbox() wrapper pattern (sandbox.ts:52-54) correctly attaches the method to avoid RPC serialization issues with WebSocket connections.

Changeset

✅ Proper changeset created for patch release.


Recommendation: Merge after addressing the error message clarity in issue #1. Issues #2-3 are minor/non-blocking.

This gets around IDEs not being able to do go-to-definition due to
mapped types not being resolved properly. This is less truthful
perhaps (hides RPC semantics), but more useful for developers.
@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 30, 2025

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/sandbox-sdk/@cloudflare/sandbox@175

commit: 25aac51

@github-actions
Copy link
Contributor

github-actions bot commented Oct 30, 2025

🐳 Docker Image Published

FROM cloudflare/sandbox:0.0.0-pr-175-5713c01

Version: 0.0.0-pr-175-5713c01

You can use this Docker image with the preview package from this PR.

@ghostwriternr ghostwriternr merged commit 77cb937 into main Oct 30, 2025
9 checks passed
@ghostwriternr ghostwriternr deleted the move-connect-to-stub branch October 30, 2025 14:17
@threepointone threepointone mentioned this pull request Oct 30, 2025
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.

2 participants