docs(http): remove unimplemented deserializeStream handler property - #641
docs(http): remove unimplemented deserializeStream handler property#641Ethan-Arrowood wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the Handler Interface documentation in reference/http/api.md by removing the deserializeStream property and updating the description of deserialize. The feedback points out that the type signature and description for serializeStream should be corrected to reflect that it serializes individual chunks and returns a buffer, Uint8Array, or string instead of a ReadableStream.
| | Property | Type | Description | | ||
| | ----------------------- | ----------------------------------------- | ----------------------------------------------------------------------------------------- | | ||
| | `serialize(data)` | `(any) => Buffer \| Uint8Array \| string` | Serialize data for a response | | ||
| | `serializeStream(data)` | `(any) => ReadableStream` | Serialize as a stream (for async iterables or large data) | |
There was a problem hiding this comment.
Based on the core ContentTypeHandler interface, serializeStream serializes individual chunks of data and returns a Buffer | string (or Buffer | Uint8Array | string), rather than returning a ReadableStream. The documented type signature and description should be updated to reflect this.
| | `serializeStream(data)` | `(any) => ReadableStream` | Serialize as a stream (for async iterables or large data) | | |
| | serializeStream(data) | (any) => Buffer | Uint8Array | string | Serialize a data chunk for a streaming response | |
References
- When documenting custom backend methods or streaming APIs, ensure that the expected return shapes or types (such as async generators and their yielded chunk structures) are explicitly specified, especially when they differ from standard wrapped return types.
There was a problem hiding this comment.
Checked this against the implementations and call sites rather than the interface declaration, and it goes the other way: serializeStream really does return a Node stream. The ContentTypeHandler declaration in server/Server.ts is stale.
The decisive evidence is the call site, server/serverHelpers/contentTypes.ts:393-425. It reaches serializeStream only when the body is an iterable or async iterable, then does stream = stream.pipe(createBrotliCompress(...)) on the return value and returns it as the response body. You cannot .pipe() a Buffer | string.
Every implementation agrees - streamAsJSON returns a JSONStream extends Readable, cbor returns an EncoderStream, msgpack and text/plain and ndjson return Readable.from(...), csv returns a piped Transform. And resources/models/v1/chatCompletions.ts:30 declares its own usage as (iterable: AsyncIterable<unknown>) => Readable.
I think the per-chunk behavior you are describing is serialize, not serializeStream. text/event-stream makes the split visible: its serializeStream is called once with the whole iterable and calls this.serialize per message internally.
So the fix went the other direction - the row now reads Readable (Node's stream, linked) and says explicitly that it is called once per response with the whole iterable, only for iterable bodies.
Worth noting server/Server.ts:100-105 is wrong on two of its four members - it also declares deserialize(data: any): Buffer | string, which is backwards for a deserializer (contentTypes.ts:26 has (data: Buffer) => unknown, and the implementations parse Buffer/string into objects). That is a harper-side fix; the docs should not follow the declaration.
sent with Claude Opus 5
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-641 This preview will update automatically when you push new commits. |
The content-type handler interface table documented a `deserializeStream(stream)` property that Harper does not implement. The `ContentTypeHandler` interface in the core repo declares only `serialize`, `serializeStream`, `deserialize`, and `q`. Removes the `deserializeStream` row and drops the now-dangling "Used when `deserializeStream` is absent." clause from the `deserialize(buffer)` description. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Handler Interface table typed `serializeStream` as returning a WHATWG `ReadableStream`. Verified against harper origin/main: every built-in handler returns a Node.js `Readable` (`streamAsJSON` -> `JSONStream extends Readable`, `Readable.from(...)`, `EncoderStream`, `toCsvStream`), and the call site in `server/serverHelpers/contentTypes.ts` pipes the return value through `createBrotliCompress()` and hands it to the HTTP layer as the response body. Also record the granularity: `serializeStream` is invoked once per response with the entire iterable, and only when the response body is an iterable or async iterable. Per-chunk serialization is `serialize`, which the streaming handlers call for each message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
e889156 to
f65c77c
Compare
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-641 This preview will update automatically when you push new commits. |
What changed
In
reference/http/api.md, the "Handler Interface" table for custom content-type handlers:deserializeStream(stream)row entirely.deserialize(buffer)description, dropping the now-dangling clause "Used whendeserializeStreamis absent." It now reads: "Deserialize an incoming request body. String fortext/*types, Buffer for binary types."No other files changed.
Why
deserializeStreamis documented but not implemented in Harper, so the table advertised a hook that silently does nothing.Source of truth verified
Checked against the
harpercore repo at commit0a727e8bd5931e9266344b757a8680f50f5980ff:grep -rn "deserializeStream" --exclude-dir=node_modules --exclude-dir=.git .returns zero matches (exit code 1).server/Server.tsdeclares the interface with exactly four members and nodeserializeStream:The three properties that remain in the docs table are all real:
serializeanddeserializeare exercised inunitTests/server/serverHelpers/contentTypes.test.js,serializeStreamis used byresources/models/v1/chatCompletions.tsand covered by SSE tests, anddeserializeis called atresources/Table.ts:5824.Scope note on v4
The original issue pointed at a 4.6 file (
versioned_docs/version-4.6/reference/globals.md). That content now lives inreference_versioned_docs/version-v4/http/api.md, which still carries both lines. PerAGENTS.md, v4 reference content is archived and frozen ("Do not modifyreference_versioned_docs/for current (v5) work"), so it is deliberately left untouched here. If the team wants the archived v4 docs corrected too, that should be a separate, explicitly-scoped change.Verification
npm run format:writethennpm run format:check- clean ("All matched files use Prettier code style!").npm run build- succeeds. Two pre-existing broken-anchor warnings remain (/reference/v5/backups/overviewand/release-notes/v5-lincoln/5.1); both are unrelated to this change and present onmain.Closes #312
🤖 Generated with Claude Code