Response Streaming in JS#3395
Conversation
Add streaming HTTP response handling to gremlin-javascript, enabling incremental result consumption via fetch response.body ReadableStream. - Add StreamReader abstraction for async byte reading over streaming and buffered sources - Refactor all ~20 GraphBinary serializers from sync deserialize(buffer) to async deserialize(reader) - Refactor GraphBinaryReader.readResponse() to use StreamReader - Add Connection.stream() using fetch response.body ReadableStream - Add Client.stream() returning AsyncGenerator - Wire Traversal API (next(), hasNext(), toList()) to streaming for incremental consumption, matching Go GLV behavior - submit() remains non-streaming, buffers full response - Remove dead readable-stream dependency and Readable imports
Cole-Greer
force-pushed
the
JS-streaming
branch
from
67d7c74 to
249285b
Compare
| try { | ||
| const deserialized = await reader.readResponse(buffer); | ||
| const attributes = new Map(); | ||
| if (deserialized.status.exception) { |
There was a problem hiding this comment.
I believe this is not the correct format of the response for the HTTP API. There are no longer any status attributes.
| * @param {Buffer} buffer | ||
| * @returns {Promise<{status: {code, message, exception}, result: {data: any[], bulked: boolean}}>} | ||
| */ | ||
| async readResponse(buffer) { |
There was a problem hiding this comment.
Just curious, is there any benefit to making GraphBinary deserialization async? You need to use the value almost immediately after calling await which is effectively synchronous?
|
Is there a test for checking that the GraphBinaryReader handles values that are input across chunk boundaries? This is technically covered by the new tests for the StreamReader, but it isn't quite the integration test that I'm expecting as it doesn't go from test input through the GraphBinary deserializers. |
GumpacG
left a comment
GumpacG
left a comment
There was a problem hiding this comment.
Could you also add an integration test that would verify early result consumption before the full response completes?
Otherwise, LGTM. VOTE +1
| import { P, TextP, Traverser } from '../../../lib/process/traversal.js'; | ||
| import { OptionsStrategy } from '../../../lib/process/traversal-strategy.js'; | ||
|
|
||
| const { anySerializer, intSerializer, longSerializer, stringSerializer, listSerializer, mapSerializer, uuidSerializer, dateTimeSerializer, floatSerializer, shortSerializer, byteSerializer, bigIntegerSerializer, binarySerializer, setSerializer, enumSerializer } = ioc; |
There was a problem hiding this comment.
Should this test file cover all serializer errors? For example, there is a missing UnspecifiedNullSerializer error.
3 participants
Add streaming HTTP response handling to gremlin-javascript, enabling incremental result consumption via fetch response.body ReadableStream.
VOTE +1