Finish RPC transport encoding levels from #144#186
Conversation
🦋 Changeset detectedLatest commit: 4b41568 The changes in this PR will be included in the next version bump. 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 |
commit: |
teamchong
force-pushed
the
review/encoding-levels-fixes
branch
from
11280c2 to
ab11478
Compare
teamchong
force-pushed
the
review/encoding-levels-fixes
branch
from
6c2dad0 to
2380156
Compare
dimitropoulos
requested review from
Copilot and
dimitropoulos
and removed request for
Copilot
There was a problem hiding this comment.
code looks good, but personally I think it could be confusing that "string" means "json string" and "json" means "objects that could be serialized to a json string - but not actually a json string". I think of json as a serialization format, and when I see "json" as an option I'd wrongly think it means what "string" actually means.
my suggestion would be:
string -> json (possibly, more clear, jsonStringified or jsonString)
json -> jsonCompatible
jsonWithBytes -> jsonCompatibleWithBytes
structuredClone -> good! but maybe there's an argument to be made for structuredClonable
|
/bonk review this |
…dundant JSON.stringify
WebSocketTransport is generic, so it can't `implements RpcTransport` directly (the ArrayBuffer instantiation intentionally doesn't conform). Replace the inline `const _typeCheck` assertion in websocket.ts with an explanatory comment, and assert the string instantiation's conformance in __type-tests__/rpc-types.test.ts instead. This matches the repo's existing type-test convention (run in CI via `test:types`), keeps the shipped source free of a dead runtime constant, and still honors the cloudflare#144 review request to separately assert WebSocketTransport<string> implements RpcTransport.
- Restore Custom transports section to match main byte-for-byte - Append suggested Custom encoding levels text verbatim - Note MessagePort wire-format break in changeset
- Narrow RpcTransport.send return to void | Promise<void> - Drop unused number return from batch transport send - Treat non-number custom send() results as unknown size - Abort session consistently on sync and async send failures - Restrict structuredClone passthrough to Date and BigInt - Export WebSocketTransport for use as a building block - Add test proving reported send() size drives flow control
teamchong
force-pushed
the
review/encoding-levels-fixes
branch
from
2380156 to
a43b006
Compare
|
/bonk review this |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Transport encoding levels (finishing #144)
I checked this out locally and verified the build and tests:
npm run test:types— passesnpm run build— succeedsnpx vitest run --project node __tests__/index.test.ts— 140 passed, including all the new encoding-level, backpressure, async-send-failure, and structured-clone stack-redaction tests.
The overall design is clean and the implementation is careful. The split between RpcTransport (string, default) and RpcTransportWithCustomEncoding, the defensive encodingLevel validation in the RpcSession constructor, and the size-estimation fallback for flow control are all well thought out and thoroughly tested. The README docs read clearly.
A few non-blocking observations below, plus one item I'd ask the maintainers to confirm.
1. Changeset semver level vs. MessagePort wire-format break
The changeset is marked minor, and the body itself notes that MessagePort sessions now post structured-clonable objects instead of JSON strings, requiring both ends to upgrade together. That's a wire-format breaking change for existing MessagePort users (a v0.x minor bump per the changeset's own warning). It's worth an explicit maintainer decision on whether this should be major/minor and whether the break is acceptable, since it's the kind of thing easy to miss in a 0.x release. (The note in the changeset is good — just flagging the policy question.)
2. Synchronous-throw send path is not directly tested
The new send() adds a queueMicrotask(() => this.abort(err, false)) branch for transports that throw synchronously (both the string and custom-encoding paths). The added test propagates async send failures from string transports only covers the rejected-promise path. The synchronous-throw branch — and the deliberate microtask deferral — has no direct coverage. Consider a small test where send() throws synchronously to lock in that behavior.
3. Minor: estimate under-counts shared (non-cyclic) substructures
In estimateEncodedSize, seen is only ever added to, never removed, so a value reachable via two different paths (shared, not cyclic) is counted in full the first time and as ESTIMATED_ENTRY_OVERHEAD (8) afterward. For flow-control estimation this is harmless (it biases low only for shared refs, and the function otherwise biases high), but it's a subtle deviation from a true serialized size. Worth a one-line comment noting this is intentional for cycle-safety rather than precise sizing.
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| "capnweb": minor | |||
There was a problem hiding this comment.
The note below correctly calls out that MessagePort sessions change wire format and both ends must upgrade together. Since that's a breaking change for existing MessagePort users, please confirm minor is the intended bump level for this (the changeset's own warning describes a minor 0.x bump). Flagging for an explicit maintainer call rather than asserting it's wrong.
|
fatal: could not read Username for 'https://github.com': No such device or address |
|
@teamchong Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
Addresses Bonk review feedback on cloudflare#186: - Add direct tests for the synchronous-throw branches of send() for both string and custom-encoding transports, locking in the queueMicrotask deferred-abort timing (previously only the async rejected-promise path was covered). - Add a comment to estimateEncodedSize() explaining that the 'seen' set is intentionally never pruned: this keeps the estimate cycle-safe at the cost of slightly under-counting shared (acyclic) substructure, which is fine for a flow-control estimate that otherwise biases high.
The Bonk action's 'Configure Git' step is designed to replace the credential that actions/checkout persists, swapping in the App installation token. With persist-credentials: false there was no credential left behind, so any git command not covered by that rewrite failed with 'could not read Username for https://github.com'. Let persist-credentials default to true (the official Bonk template relies on this); contents: read keeps the review job's token read-only.
|
bonk's working now, the run only failed on a git credential thing after the review, I included the fixes in this PR |
4 participants
Builds on @ashkalor’s PR #144 and finishes the remaining review feedback for transport encoding levels.
This keeps the original encoding-level design, with follow-up fixes for the rebase and review comments:
unknownat the transport boundary.Testing:
Original implementation by @ashkalor in #144.