refactor(sse): promote EventStream to public API, deprecate createEventStream#1509
Conversation
…ntStream Make the `EventStream` class a first-class public export and move its implementation out of `utils/internal/` into `utils/event-stream.ts`. The pure formatters and header helpers stay internal. `createEventStream` becomes a soft-deprecated compatibility wrapper and moves to `_deprecated.ts`. Docs, examples, and the migration guide now prefer `new EventStream(event)`. Co-Authored-By: Claude Opus 4.8 <[email protected]>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesEventStream API migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Co-Authored-By: Claude Opus 4.8 <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/utils/event-stream.ts (2)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
_noopto the internal helper section.Internal helpers belong at the end of the file or under
src/utils/internal/. As per coding guidelines, “Place internal helpers at the end of files or insrc/utils/internal/.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/event-stream.ts` at line 12, Move the _noop helper from its current top-level position into the file’s internal helper section at the end of the file, preserving its existing implementation and usages.Source: Coding guidelines
59-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffUse
#private fields for stream state.These fields are implementation-private but use
_names. Convert them to#fields (and update references) to follow the source convention and enforce runtime privacy. As per coding guidelines, “Usekprefixes for symbol constants,~prefixes for private/non-enumerable properties, and#for truly private class fields.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/event-stream.ts` around lines 59 - 69, Convert the implementation-private fields in the event-stream class—_event, _transformStream, _writer, _encoder, _closeCallbacks, _writerIsClosed, _paused, _unsentData, and _disposed—to # private fields, and update every reference within the class accordingly. Preserve their existing types, initializers, and behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/index.ts`:
- Around line 162-166: Update the public export block in src/index.ts to
re-export the documented isEventStream guard alongside EventStream and its
related types, so consumers can import it from the package root. Keep the
existing event-stream exports unchanged.
In `@src/utils/event-stream.ts`:
- Around line 195-203: Update flush() to detach and clear _unsentData before
awaiting _writer.write: capture the current payload in a local snapshot, reset
the pending buffer immediately, then encode and write only that snapshot.
Preserve newly pushed data for a later flush and ensure concurrent flush() calls
cannot write the same payload twice.
---
Nitpick comments:
In `@src/utils/event-stream.ts`:
- Line 12: Move the _noop helper from its current top-level position into the
file’s internal helper section at the end of the file, preserving its existing
implementation and usages.
- Around line 59-69: Convert the implementation-private fields in the
event-stream class—_event, _transformStream, _writer, _encoder, _closeCallbacks,
_writerIsClosed, _paused, _unsentData, and _disposed—to # private fields, and
update every reference within the class accordingly. Preserve their existing
types, initializers, and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d58fab8b-3130-4a68-8373-2a8c937f27b8
📒 Files selected for processing (9)
MIGRATION.mddocs/1.guide/901.advanced/2.websocket.mddocs/2.utils/2.response.mdexamples/server-sent-events.mjssrc/_deprecated.tssrc/index.tssrc/utils/event-stream.tssrc/utils/internal/event-stream.tstest/unit/sse.test.ts
| export { | ||
| type EventStreamMessage, | ||
| type EventStreamOptions, | ||
| createEventStream, | ||
| EventStream, | ||
| } from "./utils/event-stream.ts"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Export the documented isEventStream guard.
docs/2.utils/2.response.md now documents isEventStream(input), but this public export list omits it. import { isEventStream } from "h3" will fail. Re-export it here, or remove it from the public docs.
export {
type EventStreamMessage,
type EventStreamOptions,
EventStream,
+ isEventStream,
} from "./utils/event-stream.ts";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export { | |
| type EventStreamMessage, | |
| type EventStreamOptions, | |
| createEventStream, | |
| EventStream, | |
| } from "./utils/event-stream.ts"; | |
| export { | |
| type EventStreamMessage, | |
| type EventStreamOptions, | |
| EventStream, | |
| isEventStream, | |
| } from "./utils/event-stream.ts"; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` around lines 162 - 166, Update the public export block in
src/index.ts to re-export the documented isEventStream guard alongside
EventStream and its related types, so consumers can import it from the package
root. Keep the existing event-stream exports unchanged.
| async flush(): Promise<void> { | ||
| if (this._isClosed) { | ||
| return; | ||
| } | ||
| if (this._unsentData?.length) { | ||
| await this._writer.write(this._encoder.encode(this._unsentData)).catch(() => { | ||
| this._writerIsClosed = true; | ||
| }); | ||
| this._unsentData = undefined; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Detach the pending buffer before awaiting the write.
A concurrent pause() followed by push() can append to _unsentData while Line 200 awaits; Line 203 then clears that newly appended event. Concurrent flush() calls can likewise send the same payload twice. Snapshot and clear the buffer before awaiting.
Proposed fix
async flush(): Promise<void> {
if (this._isClosed) {
return;
}
- if (this._unsentData?.length) {
- await this._writer.write(this._encoder.encode(this._unsentData)).catch(() => {
+ const unsentData = this._unsentData;
+ this._unsentData = undefined;
+ if (unsentData?.length) {
+ await this._writer.write(this._encoder.encode(unsentData)).catch(() => {
this._writerIsClosed = true;
});
- this._unsentData = undefined;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async flush(): Promise<void> { | |
| if (this._isClosed) { | |
| return; | |
| } | |
| if (this._unsentData?.length) { | |
| await this._writer.write(this._encoder.encode(this._unsentData)).catch(() => { | |
| this._writerIsClosed = true; | |
| }); | |
| this._unsentData = undefined; | |
| async flush(): Promise<void> { | |
| if (this._isClosed) { | |
| return; | |
| } | |
| const unsentData = this._unsentData; | |
| this._unsentData = undefined; | |
| if (unsentData?.length) { | |
| await this._writer.write(this._encoder.encode(unsentData)).catch(() => { | |
| this._writerIsClosed = true; | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/event-stream.ts` around lines 195 - 203, Update flush() to detach
and clear _unsentData before awaiting _writer.write: capture the current payload
in a local snapshot, reset the pending buffer immediately, then encode and write
only that snapshot. Preserve newly pushed data for a later flush and ensure
concurrent flush() calls cannot write the same payload twice.
2 participants
Summary
EventStreamclass to a first-class public export (import { EventStream } from "h3").src/utils/internal/event-stream.tsinto the publicsrc/utils/event-stream.ts. The low-level, purely-internal helpers (string formatters + header helpers) remain ininternal/.createEventStreamand relocates it tosrc/_deprecated.tsas a thin compatibility wrapper aroundnew EventStream(event).docs/1.guide/901.advanced/2.websocket.md), the runnable example, andMIGRATION.mdto prefer constructing the class directly.Motivation
createEventStreamwas marked@experimentaland only ever wrappednew EventStream(event). Now that the class is stable it's cleaner to expose it directly; the wrapper stays for compatibility so this is a non-breaking change.Test plan
pnpm lint(oxlint + oxfmt) ✅npx tsc --noEmit✅pnpm vitest run test/sse.test.ts test/unit/sse.test.ts— 41 passing ✅🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
EventStreamclass for creating and managing Server-Sent Event streams.isEventStreamto identify event stream responses.EventStreamsupports pausing/resuming, buffering, pushing messages/comments, close callbacks, and automatic cleanup.Documentation
new EventStream(event).Deprecations
createEventStreamremains as a deprecated compatibility helper.Tests