Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the library to support consuming text/event-stream over an HTTP fetch() (POST) in addition to browser EventSource SSE, and updates the demo server/client to showcase the new mode.
Changes:
- Added an
HttpEventStreamStoreimplementation and a newuseTextStreamoptions overload ({ store: 'http', data, fetchInit, ... }). - Refactored the existing SSE store into a separate module and kept
EventStoreas a backwards-compatible alias. - Updated the demo server/client to provide and consume a POST-based event-stream endpoint; moved demo-running docs from README into CONTRIBUTING.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
src/hooks/useTextStream.ts |
Adds options overload + wires hook to either SSE EventStore or new HTTP store. |
src/HttpEventStreamStore.ts |
New fetch-based streaming store that parses SSE-style frames from a response body. |
src/SeverSentEventStore.ts |
Extracts prior SSE EventStore logic into a new module (with a naming typo). |
src/EventStore.ts |
Re-exports EventStore as an alias to the extracted SSE store. |
src/components/ReactTextStream/index.tsx |
Exposes store/data/fetchInit props and forwards to the hook overload. |
src/index.ts |
Exports UseTextStreamOptions type for consumers. |
demo/server/src/index.ts |
Adds /http-stream endpoint and refactors server app creation for test-friendliness. |
demo/client/src/main.tsx |
Adds a UI section demonstrating the HTTP POST + event-stream mode. |
test/integration.test.tsx |
Removes the prior integration test suite. |
README.md |
Removes local demo instructions (moved to CONTRIBUTING). |
CONTRIBUTING.md |
Adds contributor/demo workflow documentation. |
package.json / package-lock.json |
Adds dev dependencies (notably express/types) used for local/demo tooling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| while (true) { | ||
| const { done, value } = await reader.read(); | ||
| if (done) break; | ||
|
|
There was a problem hiding this comment.
When the response stream ends naturally (reader.read() returns done: true), start() exits the try without clearing controller. Since start() begins with if (controller || stopped) return;, a completed request can block any future restart attempts unless something else aborts/clears the controller. Consider clearing controller (and possibly resetting retryCount) when the stream finishes normally.
| controller?.abort(); | ||
| controller = undefined; |
There was a problem hiding this comment.
abortCurrentRequest() aborts and then sets controller = undefined. When an in-flight request is aborted (e.g., due to setData() updating input), the catch block later can’t reliably detect the abort via controller?.signal.aborted, so it will increment retryCount and may stop the stream after a few normal data changes. Keep a stable reference to the AbortController/signal for the lifetime of start() (or detect AbortError via err) so abort-driven restarts don’t count as failures.
| controller?.abort(); | |
| controller = undefined; | |
| // Keep the controller reference stable until the in-flight request finishes | |
| // unwinding so downstream error handling can still observe that it was | |
| // intentionally aborted. | |
| controller?.abort(); |
| data instanceof ArrayBuffer || | ||
| data instanceof FormData || | ||
| data instanceof URLSearchParams || | ||
| data instanceof ReadableStream |
There was a problem hiding this comment.
data instanceof ReadableStream will throw a ReferenceError in runtimes where ReadableStream isn’t defined (older browsers / some test environments). Guard with typeof ReadableStream !== 'undefined' (and similarly for other globals if needed) before using instanceof on them.
| data instanceof ReadableStream | |
| (typeof ReadableStream !== 'undefined' && data instanceof ReadableStream) |
| if (chunk !== undefined) { | ||
| setStream((curStream) => { | ||
| if (!curStream) return chunk; | ||
| return curStream.concat(chunk); | ||
| }); | ||
| } | ||
| }, [chunk]); | ||
|
|
There was a problem hiding this comment.
HttpEventStreamStore.setData() explicitly clears currentData and calls notify() when input is cleared, but this hook never clears its accumulated stream state when the store snapshot becomes undefined (it only appends when chunk !== undefined). This makes it impossible to implement a true reset/clear for the HTTP mode (and the demo’s Reset button will keep showing the old stream). Handle an undefined snapshot by resetting the accumulated stream when appropriate (e.g., when storeType === 'http' and input is cleared).
| if (chunk !== undefined) { | |
| setStream((curStream) => { | |
| if (!curStream) return chunk; | |
| return curStream.concat(chunk); | |
| }); | |
| } | |
| }, [chunk]); | |
| if (chunk === undefined) { | |
| if (storeType === 'http') { | |
| setStream(undefined); | |
| } | |
| return; | |
| } | |
| setStream((curStream) => { | |
| if (!curStream) return chunk; | |
| return curStream.concat(chunk); | |
| }); | |
| }, [chunk, storeType]); |
| useEffect(() => { | ||
| if (storeType !== 'http') return; | ||
| (eventStore as ReturnType<typeof HttpEventStreamStore<P, string, D>>).setData(data); | ||
| }, [storeType, eventStore, data]); | ||
|
|
||
| const [stream, setStream] = useState<string>(); | ||
|
|
||
| const chunk = useSyncExternalStore(eventStore.subscribe, eventStore.getSnapshot); | ||
|
|
||
| useEffect(() => { | ||
| if (chunk !== undefined) { | ||
| setStream((curStream) => { | ||
| if (!curStream) return chunk; | ||
| return curStream.concat(chunk); | ||
| }); | ||
| } | ||
| }, [chunk]); | ||
|
|
There was a problem hiding this comment.
There are no tests covering the new options overload (useTextStream({ store: 'http', data, fetchInit, ... })) or HttpEventStreamStore behavior (restarts on data change, reset on data cleared, CRLF framing, abort handling). Since this repo already has comprehensive Vitest coverage for useTextStream, please add/extend tests to exercise the HTTP store path and the new overload signature.
| useEffect(() => { | |
| if (storeType !== 'http') return; | |
| (eventStore as ReturnType<typeof HttpEventStreamStore<P, string, D>>).setData(data); | |
| }, [storeType, eventStore, data]); | |
| const [stream, setStream] = useState<string>(); | |
| const chunk = useSyncExternalStore(eventStore.subscribe, eventStore.getSnapshot); | |
| useEffect(() => { | |
| if (chunk !== undefined) { | |
| setStream((curStream) => { | |
| if (!curStream) return chunk; | |
| return curStream.concat(chunk); | |
| }); | |
| } | |
| }, [chunk]); | |
| const [stream, setStream] = useState<string>(); | |
| useEffect(() => { | |
| if (storeType !== 'http') return; | |
| setStream(undefined); | |
| (eventStore as ReturnType<typeof HttpEventStreamStore<P, string, D>>).setData(data); | |
| }, [storeType, eventStore, data]); | |
| const chunk = useSyncExternalStore(eventStore.subscribe, eventStore.getSnapshot); | |
| useEffect(() => { | |
| if (chunk === undefined) { | |
| if (storeType === 'http') { | |
| setStream(undefined); | |
| } | |
| return; | |
| } | |
| setStream((curStream) => { | |
| if (!curStream) return chunk; | |
| return curStream.concat(chunk); | |
| }); | |
| }, [chunk, storeType]); |
| const listeners = new Set(); | ||
|
|
||
| function subscribe(callback: () => void) { | ||
| listeners.add(callback); | ||
| return () => listeners.delete(callback); | ||
| } |
There was a problem hiding this comment.
listeners is currently an untyped Set, and later listeners are invoked with (listener: any) => listener(). This loses type safety and can mask incorrect listener usage. Prefer const listeners = new Set<() => void>() and iterate without any.
| function stop() { | ||
| stopped = true; | ||
| abortCurrentRequest(); | ||
| } |
There was a problem hiding this comment.
stop() sets stopped = true, which permanently disables future start() calls. Since start() calls stop() when onEvent returns undefined (used as a normal completion signal), the store can’t be restarted for a new setData() after a stream completes (and also can’t restart after all listeners briefly unsubscribe). Consider making stop() non-terminal (abort current request but allow future start()), or resetting stopped when new input is set.
| const idx = buffer.indexOf('\n\n'); | ||
| if (idx === -1) break; | ||
|
|
||
| const frame = buffer.slice(0, idx); | ||
| buffer = buffer.slice(idx + 2); |
There was a problem hiding this comment.
Frame detection only searches for "\n\n" (buffer.indexOf('\n\n')). SSE responses are also commonly delimited by CRLF ("\r\n\r\n"), in which case this loop will never find a frame boundary and the stream will stall. Consider handling both delimiters (or normalizing newlines) when scanning buffer.
| const idx = buffer.indexOf('\n\n'); | |
| if (idx === -1) break; | |
| const frame = buffer.slice(0, idx); | |
| buffer = buffer.slice(idx + 2); | |
| const lfIdx = buffer.indexOf('\n\n'); | |
| const crlfIdx = buffer.indexOf('\r\n\r\n'); | |
| let idx = -1; | |
| let delimiterLength = 0; | |
| if (lfIdx !== -1 && (crlfIdx === -1 || lfIdx < crlfIdx)) { | |
| idx = lfIdx; | |
| delimiterLength = 2; | |
| } else if (crlfIdx !== -1) { | |
| idx = crlfIdx; | |
| delimiterLength = 4; | |
| } | |
| if (idx === -1) break; | |
| const frame = buffer.slice(0, idx); | |
| buffer = buffer.slice(idx + delimiterLength); |
| buffer = buffer.slice(idx + 2); | ||
|
|
||
| for (const dataStr of parseSseEventDataFrames(frame)) { | ||
| const newData = JSON.parse(dataStr) as P; |
There was a problem hiding this comment.
JSON.parse(dataStr) inside the streaming loop will throw on any non-JSON data: payload (or partial/invalid frames) and is currently handled the same as a network error. If the API expects JSON-only, consider surfacing a clearer error or skipping malformed events so a single bad frame doesn’t tear down the stream after 3 retries.
| const newData = JSON.parse(dataStr) as P; | |
| let newData: P; | |
| try { | |
| newData = JSON.parse(dataStr) as P; | |
| } catch { | |
| continue; | |
| } |
| import { SeverSentEventStore } from './SeverSentEventStore'; | ||
|
|
||
| function subscribe(callback: () => void) { | ||
| listeners.add(callback); | ||
| return () => listeners.delete(callback); | ||
| } | ||
| // Backwards-compat alias: the project historically exported EventStore. | ||
| export const EventStore = SeverSentEventStore; |
There was a problem hiding this comment.
If you rename SeverSentEventStore/src/SeverSentEventStore.ts to fix the typo, update this import/export alias accordingly so the historical EventStore export continues to work without a breaking change.
No description provided.