docs(http): correct serializeStream return type - follow-up to #641 - #655
docs(http): correct serializeStream return type - follow-up to #641#655Ethan-Arrowood wants to merge 1 commit into
Conversation
PR #641 documented `serializeStream` as returning a Node `Readable`. That is too strict: `serialize()` selects `serializeStream` for any iterable response body, and a plain array is iterable, so handlers that cannot stream a given input return an already-serialized value instead. - `serializeStream` row now documents `Readable | Buffer | string`, matching the core `ContentTypeHandler` declaration, with a subsection covering when each occurs and the Brotli-compression caveat. - `serialize` row now includes `Readable` - the built-in `text/csv` handler's `serialize` returns a CSV transform stream. - `deserialize` row now states that Harper always passes a `Buffer`; a `text/*` handler calls `buffer.toString()` itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request updates the HTTP API Handler Interface documentation in reference/http/api.md. It refines the type definitions and descriptions for serialize, serializeStream, and deserialize, and introduces a new section explaining non-streaming returns from serializeStream. The reviewer suggested formatting the Brotli compression caveat as a :::caution block to make this critical warning more visible and maintain formatting consistency with the rest of the document.
| - The built-in `application/x-msgpack` handler streams only non-array iterables. For a plain array it returns a packed `Buffer`. | ||
| - The built-in NDJSON handler returns a string when it is handed a value that is not iterable. | ||
|
|
||
| One caveat if you return something other than a stream: when Brotli compression applies to the response (the client sent `Accept-Encoding: br` and `http.compressionThreshold` is non-zero), Harper pipes the value returned by `serializeStream` into the compressor. A `Buffer` or string has no `pipe`, so a handler that must work with compression enabled should return a stream — wrap a buffered result with `Readable.from([buffer])` if there is nothing to stream incrementally. |
There was a problem hiding this comment.
Since this Brotli compression caveat describes a scenario that leads to a runtime crash (TypeError: stream.pipe is not a function), it is highly recommended to format it using a :::caution block. This aligns with the established formatting patterns on this page (such as on lines 159 and 186) and ensures that developers scanning the documentation do not miss this critical warning.
| One caveat if you return something other than a stream: when Brotli compression applies to the response (the client sent `Accept-Encoding: br` and `http.compressionThreshold` is non-zero), Harper pipes the value returned by `serializeStream` into the compressor. A `Buffer` or string has no `pipe`, so a handler that must work with compression enabled should return a stream — wrap a buffered result with `Readable.from([buffer])` if there is nothing to stream incrementally. | |
| :::caution | |
| One caveat if you return something other than a stream: when Brotli compression applies to the response (the client sent Accept-Encoding: br and http.compressionThreshold is non-zero), Harper pipes the value returned by serializeStream into the compressor. A Buffer or string has no pipe, so a handler that must work with compression enabled should return a stream — wrap a buffered result with Readable.from([buffer]) if there is nothing to stream incrementally. | |
| ::: |
References
- When breaking down long, dense paragraphs or formatting warnings in Markdown documentation, maintain consistency with the existing formatting patterns of the page.
🚀 Preview DeploymentYour preview deployment is ready! 🔗 Preview URL: https://preview.harper-documentation.harperfabric.com/pr-655 This preview will update automatically when you push new commits. |
kriszyp
left a comment
There was a problem hiding this comment.
We could probably go deeper with the return type, but I think this covers most of the cases.
🤖 Reviewed with Codex
| - The built-in `application/x-msgpack` handler streams only non-array iterables. For a plain array it returns a packed `Buffer`. | ||
| - The built-in NDJSON handler returns a string when it is handed a value that is not iterable. | ||
|
|
||
| One caveat if you return something other than a stream: when Brotli compression applies to the response (the client sent `Accept-Encoding: br` and `http.compressionThreshold` is non-zero), Harper pipes the value returned by `serializeStream` into the compressor. A `Buffer` or string has no `pipe`, so a handler that must work with compression enabled should return a stream — wrap a buffered result with `Readable.from([buffer])` if there is nothing to stream incrementally. |
There was a problem hiding this comment.
This documents a framework bug as a handler requirement. Buffer and string are now advertised as supported serializeStream results, but a routine request with Accept-Encoding: br can fail because the compressor assumes .pipe(). With the default compression threshold and browser clients, custom handlers cannot reliably use part of the documented API. Harper should normalize a non-stream result with Readable.from([value]) before the compression branch, so the return contract is independent of a response-header/configuration combination; then this caveat can be removed or reduced to normal streaming guidance.
| | Property | Type | Description | | ||
| | ----------------------- | ----------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `serialize(data)` | `(any) => Buffer \| Uint8Array \| string \| Readable` | Serialize a complete response body. Used when the response body is not iterable, or when the handler defines no `serializeStream`. Most handlers return a string or a `Buffer`; the built-in `text/csv` handler returns a [`Readable`](https://nodejs.org/api/stream.html#class-streamreadable), which Harper streams to the response. | | ||
| | `serializeStream(data)` | `(any) => Readable \| Buffer \| string` | Serialize a streaming response body. Called once per response with the whole iterable (not once per chunk), and only when the response body is an object that is iterable or async iterable. A Node.js [`Readable`](https://nodejs.org/api/stream.html#class-streamreadable) is the usual return, but it is not the only one — see [Non-streaming returns from `serializeStream`](#non-streaming-returns-from-serializestream). | |
There was a problem hiding this comment.
The return type is incomplete relative to the subsection immediately below it: serializeStream is said to accept any iterable or async iterable and wrap it with Readable.from(), but neither is represented here. A typed custom handler returning a generator would therefore be rejected despite the documented runtime behavior. Add the iterable forms to the public type (and its core declaration), or narrow the prose to the supported contract.
| The union is not theoretical. Harper picks `serializeStream` over `serialize` whenever the response body is iterable, and a plain array is iterable — so a resource that returns an array reaches `serializeStream`. A handler that has nothing to stream in that case can serialize the value in one shot instead: | ||
|
|
||
| - The built-in `application/x-msgpack` handler streams only non-array iterables. For a plain array it returns a packed `Buffer`. | ||
| - The built-in NDJSON handler returns a string when it is handed a value that is not iterable. |
There was a problem hiding this comment.
This is not a reachable serializeStream dispatch case: the preceding contract says Harper selects this method only when the response body is iterable or async iterable. A non-iterable response instead uses serialize, so this example does not demonstrate the new return union and can mislead readers about which method runs. Remove it, or explicitly frame it as a direct call outside Harper's normal response path.
Follow-up to #641, which is already merged. @kriszyp reviewed it after the merge and is right that the row it added is too strict:
Why #641 was wrong
#641 assumed the
Buffer | stringreturn paths were unreachable "since the callers gate on iterability first." That reasoning does not hold: an array is iterable, so the gate does not exclude it.server/serverHelpers/contentTypes.ts:393-398(harperorigin/main):A plain array satisfies
typeof === 'object'and hasSymbol.iterator, so an array response reachesserializeStream, notserialize.Evidence, per registered handler
Verified against
HarperFast/harperatorigin/main(ad9854bed).mediaTypesmap:serializeStreamreturnapplication/json,*/*,''contentTypes.ts:44,:194streamAsJSON->JSONStream extends Readable(JSONStream.ts:12-16). Always aReadable.application/cborcontentTypes.ts:53-56new EncoderStream(...).end(data)-> the transform stream. Always a stream.application/x-msgpackcontentTypes.ts:62-67ReadableorBuffer. Explicit&& !Array.isArray(data)guard; an array falls through toreturn pack(data).text/csvcontentTypes.ts:73-76toCsvStream->readStream.pipe(csvTransform)(:702). Always a stream.text/plaincontentTypes.ts:88-90Readable.from(...). Always aReadable.application/x-ndjson,application/ndjsoncontentTypes.ts:106-111Readableorstring. Non-iterable input returnsJSONStringify(data) + '\n'.text/event-streamcontentTypes.ts:135-138Readable.from(...). Always aReadable.Fastify serializer list (
registerContentHandlers,contentTypes.ts:211-241): same shapes - the msgpack entry at:226-231carries the identical!Array.isArray(data)guard andreturn pack(data).Custom handlers may also return a bare iterator:
unitTests/testApp/resources.js:258-269registersserializeStreamas a generator / async generator, exercised byunitTests/apiTests/basicREST-test.mjs:587-611.server/http.ts:741-742wraps a non-Readableiterable withReadable.from(), and:745-751sends aBuffer/string body directly, so the whole union really is written to the response.The core declaration agrees -
server/Server.ts:100-105:The compression branch is a latent bug
contentTypes.ts:410-425pipes theserializeStreamresult unconditionally when compression is active:A
Bufferhas no.pipe, soAccept: application/x-msgpack+Accept-Encoding: br+ an array response body throwsTypeError: stream.pipe is not a functionwhenhttp.compressionThresholdis non-zero (canCompressis gated on it at:373). The union is real, but that one branch does not currently handle it. Not fixed here - this is a docs PR - but the docs now warn custom-handler authors to return a stream if their handler may run with compression enabled, and it is worth a core issue.Changes
serializeStreamrow:(any) => Readable | Buffer | string, plus a new Non-streaming returns fromserializeStreamsubsection covering when each form occurs, that iterables are accepted too, and the compression caveat.serializerow: addedReadable. The built-intext/csvhandler's non-streamingserialize(contentTypes.ts:77-81) also returnstoCsvStream(...), a stream - so that row was over-strict in the same way.deserializerow: was(Buffer | string) => anywith "String fortext/*types, Buffer for binary types." Harper always hands the handler aBuffer-getDeserializerstreams to a buffer (contentTypes.ts:590), MQTT passespacket.payload(server/mqtt.ts:515), and the registeredtext/plainhandler callsdata.toString()itself (contentTypes.ts:91-93).Verification
npm run format:write/npm run format:checkclean.npm run buildsucceeds with zero broken-link or broken-anchor warnings; the new#non-streaming-returns-from-serializestreamanchor resolves in the built HTML.🤖 Generated with Claude Code