Fix/web stream error handling#14732
Conversation
Removes code duplication between runtime-core and server-renderer by introducing a shared getResolvedCompilerOptions helper. This addresses a TODO in ssrCompile.ts and improves maintainability.
📝 WalkthroughWalkthroughRefactored runtime compiler-options resolution into an exported helper Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App (VNode)
participant Renderer as SSR Renderer
participant Buffer as SimpleReadable/Buffer
participant Writer as Writable (Node/Web)
App->>Renderer: render vnode (async)
Renderer->>Buffer: push(chunk)
alt push returns Promise
Buffer->>Renderer: promise resolves (backpressure)
end
Renderer->>Writer: write/push to destination
alt destination reports backpressure
Writer-->>Renderer: write() returns false / desiredSize <= 0
Renderer->>Writer: await drain / pull to resume
end
alt render throws
Renderer->>Writer: destroy(err)
Writer->>Writer: abort(err) (web) / destroy(err) (node)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/server-renderer/src/renderToStream.ts (2)
17-17: LGTM — async-aware unrolling is required for the WebWritable push.Since
pipeToWebWritable'spushawaitswriter.readyand returnswriter.write(...)/writer.close()promises, the old fire-and-forgetstream.push(...)could lose back-pressure and, more importantly, swallow write-side rejections (e.g. after an abort). Awaitingpushand propagating async pauses throughunrollBufferSyncvia thebuffer.slice(i + 1)continuation correctly funnels any write rejection back intorenderToSimpleStream's.catch, which in turn triggersdestroy(err).Minor nit (optional):
buffer.slice(i + 1)allocates a new array on each async boundary; an index parameter would avoid that if this ever becomes a hot path, but given async pauses are rare this is fine as-is.Also applies to: 32-40, 44-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-renderer/src/renderToStream.ts` at line 17, The WebWritable push implementation used by pipeToWebWritable must be async-aware: change the push signature/usage so that push returns/awaits the Promise from writer.ready followed by writer.write(...) or writer.close(), and update unrollBufferSync to await stream.push(...) and pass the remaining work via buffer.slice(i + 1) (or an index-based continuation) so any write rejections propagate back into renderToSimpleStream's .catch and trigger destroy(err); locate implementations named push, pipeToWebWritable, unrollBufferSync, renderToSimpleStream and replace fire-and-forget stream.push(...) calls with awaited pushes that return the writer promises.
219-223: LGTM —abortnow surfaces the SSR error to consumers.Switching from
console.log + writer.close()towriter.abort(err)correctly propagates the failure to the downstreamWritableStream(matching the new test). The.catch(() => {})is appropriate because:
abort()rejects if the writer is already closed/errored (e.g. a prior failedwriter.write(...)already errored the stream and bubbled up todestroy);- without the catch, that would become an unhandled rejection.
One small consideration: silently swallowing the abort rejection also hides unexpected failures (e.g. a sink
abort()that itself throws for reasons unrelated to an already-errored stream). If you want visibility in dev, consider a__DEV__-gatedwarn:💡 Optional: surface unexpected abort failures in dev
destroy(err) { - writer.abort(err).catch(() => { - // ignore errors from aborting an already closed/errored stream - }) + writer.abort(err).catch(e => { + if (__DEV__) { + // eslint-disable-next-line no-console + console.warn(`[`@vue/server-renderer`] writer.abort() failed:`, e) + } + }) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-renderer/src/renderToStream.ts` around lines 219 - 223, The destroy implementation swallows all abort rejections which can hide unexpected sink failures; update the destroy method (the destroy function that calls writer.abort(err).catch(...)) to capture the abort rejection error (e.g., .catch((abortErr) => { ... })) and, inside the catch, rethrow or ignore only the expected "already closed/errored" case but surface other failures in development by logging a warning gated by a dev flag (e.g., __DEV__ or process.env.NODE_ENV !== 'production') so writer.abort(err) still avoids unhandled rejections but unexpected abort errors are visible during development.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server-renderer/__tests__/webStream.spec.ts`:
- Around line 82-88: The test uses a fixed 10ms setTimeout to wait for the abort
which is flaky; replace the timeout with an event-driven promise that resolves
when the Writable's abort handler runs so the test waits exactly for the abort.
Specifically, in the test that calls pipeToWebWritable(createApp(App), {},
writable) replace the sleep with a Promise whose resolver is called from the
writable.abort (or the assigned abort callback) where you already set
abortedReason; ensure the promise resolves inside that abort handler so the
subsequent expects (abortedReason and its message) run only after abort() has
executed.
---
Nitpick comments:
In `@packages/server-renderer/src/renderToStream.ts`:
- Line 17: The WebWritable push implementation used by pipeToWebWritable must be
async-aware: change the push signature/usage so that push returns/awaits the
Promise from writer.ready followed by writer.write(...) or writer.close(), and
update unrollBufferSync to await stream.push(...) and pass the remaining work
via buffer.slice(i + 1) (or an index-based continuation) so any write rejections
propagate back into renderToSimpleStream's .catch and trigger destroy(err);
locate implementations named push, pipeToWebWritable, unrollBufferSync,
renderToSimpleStream and replace fire-and-forget stream.push(...) calls with
awaited pushes that return the writer promises.
- Around line 219-223: The destroy implementation swallows all abort rejections
which can hide unexpected sink failures; update the destroy method (the destroy
function that calls writer.abort(err).catch(...)) to capture the abort rejection
error (e.g., .catch((abortErr) => { ... })) and, inside the catch, rethrow or
ignore only the expected "already closed/errored" case but surface other
failures in development by logging a warning gated by a dev flag (e.g., __DEV__
or process.env.NODE_ENV !== 'production') so writer.abort(err) still avoids
unhandled rejections but unexpected abort errors are visible during development.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e787612f-af70-48ea-99f2-b2aae40dc395
📒 Files selected for processing (5)
packages/runtime-core/src/component.tspackages/runtime-core/src/index.tspackages/server-renderer/__tests__/webStream.spec.tspackages/server-renderer/src/helpers/ssrCompile.tspackages/server-renderer/src/renderToStream.ts
| pipeToWebWritable(createApp(App), {}, writable) | ||
|
|
||
| // Wait for the error to propagate | ||
| await new Promise(resolve => setTimeout(resolve, 10)) | ||
|
|
||
| expect(abortedReason).toBeInstanceOf(Error) | ||
| expect(abortedReason.message).toBe('ssr render error') |
There was a problem hiding this comment.
Flaky wait — prefer event-driven synchronization over a fixed 10 ms timeout.
setTimeout(resolve, 10) is a race; under CI load (slow machines, GC, microtask backlog) the abort may not have propagated yet, producing false negatives (abortedReason still undefined). Use a promise resolved from inside abort():
🧪 Proposed fix
let abortedReason: any
+ let resolveAbort!: () => void
+ const aborted = new Promise<void>(r => (resolveAbort = r))
const writable = new WritableStream({
abort(reason) {
abortedReason = reason
+ resolveAbort()
},
})
pipeToWebWritable(createApp(App), {}, writable)
- // Wait for the error to propagate
- await new Promise(resolve => setTimeout(resolve, 10))
+ await aborted
expect(abortedReason).toBeInstanceOf(Error)
expect(abortedReason.message).toBe('ssr render error')📝 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.
| pipeToWebWritable(createApp(App), {}, writable) | |
| // Wait for the error to propagate | |
| await new Promise(resolve => setTimeout(resolve, 10)) | |
| expect(abortedReason).toBeInstanceOf(Error) | |
| expect(abortedReason.message).toBe('ssr render error') | |
| let abortedReason: any | |
| let resolveAbort!: () => void | |
| const aborted = new Promise<void>(r => (resolveAbort = r)) | |
| const writable = new WritableStream({ | |
| abort(reason) { | |
| abortedReason = reason | |
| resolveAbort() | |
| }, | |
| }) | |
| pipeToWebWritable(createApp(App), {}, writable) | |
| await aborted | |
| expect(abortedReason).toBeInstanceOf(Error) | |
| expect(abortedReason.message).toBe('ssr render error') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server-renderer/__tests__/webStream.spec.ts` around lines 82 - 88,
The test uses a fixed 10ms setTimeout to wait for the abort which is flaky;
replace the timeout with an event-driven promise that resolves when the
Writable's abort handler runs so the test waits exactly for the abort.
Specifically, in the test that calls pipeToWebWritable(createApp(App), {},
writable) replace the sleep with a Promise whose resolver is called from the
writable.abort (or the assigned abort callback) where you already set
abortedReason; ensure the promise resolves inside that abort handler so the
subsequent expects (abortedReason and its message) run only after abort() has
executed.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/runtime-core/src/component.ts (1)
1046-1055:⚠️ Potential issue | 🔴 CriticalStream changes are included in this PR; focus verification on writer lock cleanup.
The stream-related changes referenced (async push handling, writer.abort() on render errors, and error handling tests) are present in the current code at
packages/server-renderer/src/renderToStream.tsand include the new test filewebStream.spec.ts. Error propagation from synchronous render correctly flows throughstream.destroy()towriter.abort(err), as verified by the error handling test.However, the implementation has a critical issue:
pipeToWebWritablecallswritable.getWriter()on line 199 but never callsreleaseLock()to release the writer lock. This prevents proper stream cleanup and can block future operations on the stream. Wrap the entire render operation intry/finallyto ensurewriter.releaseLock()is called even ifwriter.abort()rejects:const writer = writable.getWriter() try { renderToSimpleStream(input, context, { /* ... */ }) } finally { writer.releaseLock() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/runtime-core/src/component.ts` around lines 1046 - 1055, pipeToWebWritable acquires a writer via writable.getWriter() but never releases its lock, which can block stream cleanup; update the implementation in pipeToWebWritable to wrap the render operation (the call to renderToSimpleStream / stream writing logic) in a try/finally where you always call writer.releaseLock() in the finally block so the lock is released even if writer.abort(err) or renderToSimpleStream throws/rejects; ensure you still call writer.abort(err) on errors before releasing the lock so error propagation remains intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/runtime-core/src/component.ts`:
- Around line 1046-1055: pipeToWebWritable acquires a writer via
writable.getWriter() but never releases its lock, which can block stream
cleanup; update the implementation in pipeToWebWritable to wrap the render
operation (the call to renderToSimpleStream / stream writing logic) in a
try/finally where you always call writer.releaseLock() in the finally block so
the lock is released even if writer.abort(err) or renderToSimpleStream
throws/rejects; ensure you still call writer.abort(err) on errors before
releasing the lock so error propagation remains intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48d27b56-a0bc-4a7f-974f-f89014806fd0
📒 Files selected for processing (2)
packages/runtime-core/src/component.tspackages/runtime-core/src/index.ts
|
Thanks for the PR. This one includes changes of #14731. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/server-renderer/__tests__/nodeStream.spec.ts (1)
49-67: Rename this test or add actualrenderToNodeStreambackpressure coverage.This currently verifies async SSR output only; it never forces
Readable.push()to returnfalse, so the newrenderToNodeStreambackpressure branch can regress without this test failing.Minimal rename if full backpressure coverage is deferred
- test('renderToNodeStream backpressure', async () => { + test('renderToNodeStream streams async SSR output', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-renderer/__tests__/nodeStream.spec.ts` around lines 49 - 67, Rename the test 'renderToNodeStream backpressure' (or add true backpressure simulation) because it currently only verifies async SSR output and never forces Readable.push() to return false; update the test around renderToNodeStream/createApp(App) to either change the test name to reflect it's async SSR-only (e.g., 'renderToNodeStream async SSR output') or modify the test to simulate Node Readable backpressure by creating a custom Readable consumer that limits its internal buffer (or mocks push() to return false) and asserts that the renderToNodeStream backpressure branch is exercised when iterating the stream variable.packages/server-renderer/src/renderToStream.ts (1)
50-68: Avoid slicing buffers on every async resume.Under sustained backpressure,
buffer.slice(i + 1)repeatedly copies the remaining SSR buffer. Track the next index instead to keep the resume path allocation-light.Proposed allocation-light resume
function unrollBufferSync( buffer: SSRBuffer, stream: SimpleReadable, + start = 0, ): void | Promise<void> { - for (let i = 0; i < buffer.length; i++) { + for (let i = start; i < buffer.length; i++) { let item = buffer[i] if (isString(item)) { const res = stream.push(item) if (isPromise(res)) { // if the stream is async, we can't unroll it syncly anymore // this can happen if a sync buffer is being pushed to an async stream - return res.then(() => unrollBufferSync(buffer.slice(i + 1), stream)) + return res.then(() => unrollBufferSync(buffer, stream, i + 1)) } } else { // since this is a sync buffer, child buffers are never promises const res = unrollBufferSync(item as SSRBuffer, stream) if (isPromise(res)) { - return res.then(() => unrollBufferSync(buffer.slice(i + 1), stream)) + return res.then(() => unrollBufferSync(buffer, stream, i + 1)) } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server-renderer/src/renderToStream.ts` around lines 50 - 68, The unrollBufferSync function currently slices the SSRBuffer on every async resume which causes repeated allocations; change the algorithm to resume by tracking the next index instead of calling buffer.slice(i + 1). Modify unrollBufferSync (or add an inner helper) to accept a startIndex parameter (e.g., unrollBufferSync(buffer, stream, startIndex)) and use that index to continue iteration after a promise resolution; when returning a promise, chain a .then that calls the same function with the original buffer and the next index rather than creating a slice. Ensure both string and nested-buffer branches use the index-based resume so no buffer copying occurs on backpressure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server-renderer/__tests__/nodeStream.spec.ts`:
- Around line 18-44: The test races because calling originalWrite immediately
lets Node emit a real drain; change the writable.write override (the one using
originalWrite, firstCall and writeCount) so that on the first call you call
originalWrite(chunk, encoding, wrappedCb) where wrappedCb saves the real
callback into a local heldCb variable but does NOT invoke it, and return false;
after the expect(writeCount).toBe(1) invoke heldCb() and then emit('drain') on
writable (keeping the existing writable.emit('drain') call) so the drain is
triggered deterministically.
In `@packages/server-renderer/src/renderToStream.ts`:
- Around line 149-164: The inline callbacks passed to renderToSimpleStream are
missing parameter type annotations causing implicit any; update the push and
destroy signatures to match SimpleReadable: change push(content) to
push(content: string | null) and destroy(err: any) to destroy(err: any), remove
the trailing "as any" cast on the options object, and ensure resolveRead remains
a Promise<void> resolver so the returned Promise type stays correct (references:
renderToSimpleStream, push, destroy, stream, resolveRead in renderToStream.ts).
- Around line 16-20: The waitDrain(stream: Writable) promise can hang if the
writable emits 'error'/'close'/'finish'/'destroy' before 'drain'; update
waitDrain to attach listeners for 'drain', 'error', 'close' and 'finish' (or
'close'/'destroy') that either resolve on 'drain' or reject on terminal events,
and ensure all listeners are removed in the cleanup path to avoid leaks; apply
the same lifecycle-safe pattern to the other identical drain-wait implementation
referenced (the block around lines 175-179) so both places reject promptly if
the destination errors/closes.
---
Nitpick comments:
In `@packages/server-renderer/__tests__/nodeStream.spec.ts`:
- Around line 49-67: Rename the test 'renderToNodeStream backpressure' (or add
true backpressure simulation) because it currently only verifies async SSR
output and never forces Readable.push() to return false; update the test around
renderToNodeStream/createApp(App) to either change the test name to reflect it's
async SSR-only (e.g., 'renderToNodeStream async SSR output') or modify the test
to simulate Node Readable backpressure by creating a custom Readable consumer
that limits its internal buffer (or mocks push() to return false) and asserts
that the renderToNodeStream backpressure branch is exercised when iterating the
stream variable.
In `@packages/server-renderer/src/renderToStream.ts`:
- Around line 50-68: The unrollBufferSync function currently slices the
SSRBuffer on every async resume which causes repeated allocations; change the
algorithm to resume by tracking the next index instead of calling buffer.slice(i
+ 1). Modify unrollBufferSync (or add an inner helper) to accept a startIndex
parameter (e.g., unrollBufferSync(buffer, stream, startIndex)) and use that
index to continue iteration after a promise resolution; when returning a
promise, chain a .then that calls the same function with the original buffer and
the next index rather than creating a slice. Ensure both string and
nested-buffer branches use the index-based resume so no buffer copying occurs on
backpressure.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb7c17a5-4782-40b4-92ef-9b7a3fda5867
📒 Files selected for processing (3)
packages/server-renderer/__tests__/nodeStream.spec.tspackages/server-renderer/__tests__/webStream.spec.tspackages/server-renderer/src/renderToStream.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server-renderer/tests/webStream.spec.ts
| const writable = new Writable({ | ||
| highWaterMark: 1, | ||
| write(_chunk, _encoding, callback) { | ||
| writeCount++ | ||
| callback() | ||
| }, | ||
| }) | ||
|
|
||
| const originalWrite = writable.write.bind(writable) | ||
| let firstCall = true | ||
| writable.write = (chunk: any, encoding?: any, cb?: any): any => { | ||
| if (firstCall) { | ||
| firstCall = false | ||
| originalWrite(chunk, encoding, cb) | ||
| return false | ||
| } | ||
| return originalWrite(chunk, encoding, cb) | ||
| } | ||
|
|
||
| pipeToNodeWritable(createApp(App), {}, writable) | ||
|
|
||
| await new Promise(resolve => setTimeout(resolve, 20)) | ||
| // Should have only 1 write because it returned false and we're waiting for drain | ||
| expect(writeCount).toBe(1) | ||
|
|
||
| writable.emit('drain') | ||
| await new Promise(resolve => setTimeout(resolve, 20)) |
There was a problem hiding this comment.
Make the backpressure test control drain deterministically.
Calling the original Writable.write() and immediately invoking its callback lets Node emit a real drain before Line 43, so the Line 41 assertion can race and fail before the manual drain.
Proposed deterministic gate
let writeCount = 0
+ let unblockFirstWrite: (() => void) | undefined
const writable = new Writable({
highWaterMark: 1,
write(_chunk, _encoding, callback) {
writeCount++
+ if (writeCount === 1) {
+ unblockFirstWrite = callback
+ return
+ }
callback()
},
})
-
- const originalWrite = writable.write.bind(writable)
- let firstCall = true
- writable.write = (chunk: any, encoding?: any, cb?: any): any => {
- if (firstCall) {
- firstCall = false
- originalWrite(chunk, encoding, cb)
- return false
- }
- return originalWrite(chunk, encoding, cb)
- }
pipeToNodeWritable(createApp(App), {}, writable)
await new Promise(resolve => setTimeout(resolve, 20))
// Should have only 1 write because it returned false and we're waiting for drain
expect(writeCount).toBe(1)
+ expect(unblockFirstWrite).toBeDefined()
+ unblockFirstWrite!()
writable.emit('drain')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server-renderer/__tests__/nodeStream.spec.ts` around lines 18 - 44,
The test races because calling originalWrite immediately lets Node emit a real
drain; change the writable.write override (the one using originalWrite,
firstCall and writeCount) so that on the first call you call
originalWrite(chunk, encoding, wrappedCb) where wrappedCb saves the real
callback into a local heldCb variable but does NOT invoke it, and return false;
after the expect(writeCount).toBe(1) invoke heldCb() and then emit('drain') on
writable (keeping the existing writable.emit('drain') call) so the drain is
triggered deterministically.
| function waitDrain(stream: Writable): Promise<void> { | ||
| return new Promise(resolve => { | ||
| stream.once('drain', resolve) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -e ts 'renderToStream' packages/Repository: vuejs/core
Length of output: 100
🏁 Script executed:
cat -n packages/server-renderer/src/renderToStream.ts | head -200Repository: vuejs/core
Length of output: 6416
🏁 Script executed:
cat -n packages/server-renderer/src/renderToStream.ts | sed -n '169,188p'Repository: vuejs/core
Length of output: 619
🏁 Script executed:
# Check if there's any stream error handling in the calling code
rg -A 5 'pipeToNodeWritable' packages/server-renderer/src/ --type tsRepository: vuejs/core
Length of output: 1303
🏁 Script executed:
# Check Node.js stream behavior documentation context and any tests
fd -e test -o -e spec packages/server-renderer/ | head -5Repository: vuejs/core
Length of output: 170
🏁 Script executed:
fd test packages/server-renderer | head -10Repository: vuejs/core
Length of output: 89
🏁 Script executed:
# Look for test files with different patterns
find packages/server-renderer -name '*.spec.ts' -o -name '*.test.ts' | head -10Repository: vuejs/core
Length of output: 629
🏁 Script executed:
# Check if there's any error handling or stream monitoring in the broader context
rg 'on\(.*error' packages/server-renderer/src/ --type tsRepository: vuejs/core
Length of output: 36
🏁 Script executed:
cat -n packages/server-renderer/__tests__/nodeStream.spec.ts | head -100Repository: vuejs/core
Length of output: 2576
🏁 Script executed:
# Check if there are any tests for error handling in pipeToNodeWritable
rg 'error|destroy' packages/server-renderer/__tests__/nodeStream.spec.ts -A 3Repository: vuejs/core
Length of output: 36
🏁 Script executed:
# Check if there's any documentation about stream error handling
rg -l 'pipeToNodeWritable' packages/server-renderer/__tests__/ | xargs grep -l 'error\|destroy' 2>/dev/null || echo "No error handling tests found"Repository: vuejs/core
Length of output: 103
🏁 Script executed:
# Verify Node.js stream behavior: destroyed streams don't emit drain
# Let's check the TypeScript types to understand stream lifecycle
rg 'interface.*Writable\|type.*Writable' packages/server-renderer/src/renderToStream.ts -B 2 -A 5Repository: vuejs/core
Length of output: 36
🏁 Script executed:
# Check Node.js stream type definitions to confirm destroyed property and event behavior
rg 'destroyed|drain|error.*event' packages/server-renderer/src/renderToStream.tsRepository: vuejs/core
Length of output: 87
🏁 Script executed:
# Look at imports to see TypeScript version
cat packages/server-renderer/tsconfig.json | head -30Repository: vuejs/core
Length of output: 124
🏁 Script executed:
# Check the full renderToSimpleStream promise chain to see error handling
cat -n packages/server-renderer/src/renderToStream.ts | sed -n '93,110p'Repository: vuejs/core
Length of output: 607
🏁 Script executed:
# Look at pipeToNodeWritable again - does the Promise.reject ever get handled?
cat -n packages/server-renderer/src/renderToStream.ts | sed -n '169,188p'Repository: vuejs/core
Length of output: 619
🏁 Script executed:
# Check if renderToSimpleStream catches any promise rejections from the push method
cat -n packages/server-renderer/src/renderToStream.ts | sed -n '93,110p'Repository: vuejs/core
Length of output: 607
🏁 Script executed:
# Verify that if push() rejects, it's caught by the catch handler
# The push() is called in unrollBuffer via stream.push() at line 38
cat -n packages/server-renderer/src/renderToStream.ts | sed -n '27,48p'Repository: vuejs/core
Length of output: 737
🏁 Script executed:
# Double-check: the async push in pipeToNodeWritable is awaited, right?
cat -n packages/server-renderer/src/renderToStream.ts | sed -n '73,110p'Repository: vuejs/core
Length of output: 1261
🏁 Script executed:
# Check what happens if a writable is destroyed - does it still emit drain?
# Let's look for any existing stream error handling patterns in the codebase
rg 'on.*error\|on.*close\|on.*destroy' packages/server-renderer/src/ --type ts -B 2 -A 2Repository: vuejs/core
Length of output: 36
🏁 Script executed:
# Verify the proposed fix is sound by checking if Node.js Writable has these events
# Check if there are examples of proper stream error handling elsewhere in the repo
rg 'removeListener\|once.*error' packages/ --type ts | head -10Repository: vuejs/core
Length of output: 36
Handle writable error/close while waiting for drain.
If write() returns false and the destination errors, closes, or is destroyed before drain emits, await waitDrain(writable) never settles and SSR rendering stalls indefinitely. The error handling in the rendering pipeline depends on promise rejection, which cannot occur if the promise hangs.
Proposed lifecycle-safe drain wait
function waitDrain(stream: Writable): Promise<void> {
- return new Promise(resolve => {
- stream.once('drain', resolve)
+ if (stream.destroyed) {
+ return Promise.reject(new Error('Writable stream was destroyed before drain'))
+ }
+
+ return new Promise((resolve, reject) => {
+ const cleanup = () => {
+ stream.removeListener('drain', onDrain)
+ stream.removeListener('error', onError)
+ stream.removeListener('close', onClose)
+ }
+ const onDrain = () => {
+ cleanup()
+ resolve()
+ }
+ const onError = (err: Error) => {
+ cleanup()
+ reject(err)
+ }
+ const onClose = () => {
+ cleanup()
+ reject(new Error('Writable stream closed before drain'))
+ }
+
+ stream.once('drain', onDrain)
+ stream.once('error', onError)
+ stream.once('close', onClose)
})
}Also applies to: 175-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server-renderer/src/renderToStream.ts` around lines 16 - 20, The
waitDrain(stream: Writable) promise can hang if the writable emits
'error'/'close'/'finish'/'destroy' before 'drain'; update waitDrain to attach
listeners for 'drain', 'error', 'close' and 'finish' (or 'close'/'destroy') that
either resolve on 'drain' or reject on terminal events, and ensure all listeners
are removed in the cleanup path to avoid leaks; apply the same lifecycle-safe
pattern to the other identical drain-wait implementation referenced (the block
around lines 175-179) so both places reject promptly if the destination
errors/closes.
| renderToSimpleStream(input, context, { | ||
| push(content) { | ||
| if (content != null) { | ||
| if (!stream.push(content)) { | ||
| return new Promise<void>(resolve => { | ||
| resolveRead = resolve | ||
| }) | ||
| } | ||
| } else { | ||
| stream.push(null) | ||
| } | ||
| }, | ||
| destroy(err) { | ||
| stream.destroy(err) | ||
| }, | ||
| } as any) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/server-renderer/src/renderToStream.ts | sed -n '140,170p'Repository: vuejs/core
Length of output: 938
🏁 Script executed:
# Let's understand the renderToSimpleStream signature and the expected type
rg -A 20 "function renderToSimpleStream|const renderToSimpleStream" packages/server-renderer/src/Repository: vuejs/core
Length of output: 1574
🏁 Script executed:
# Check if there's a type definition for the options parameter
rg -B 5 -A 15 "renderToSimpleStream" packages/server-renderer/src/renderToSimpleStream.ts | head -100Repository: vuejs/core
Length of output: 146
🏁 Script executed:
rg -B 3 -A 10 "type SimpleReadable|interface SimpleReadable" packages/server-renderer/src/Repository: vuejs/core
Length of output: 1585
🏁 Script executed:
# Also check if there are any type definitions imported or defined in renderToStream.ts
head -50 packages/server-renderer/src/renderToStream.tsRepository: vuejs/core
Length of output: 1228
Fix the TypeScript implicit any failures.
The inline push/destroy methods lack contextual typing, causing TypeScript to infer any for the parameter types. According to the SimpleReadable interface, push expects chunk: string | null and destroy expects err: any. The as any cast currently suppresses these type errors.
Add the missing parameter type annotations and remove the as any cast:
Proposed fix
renderToSimpleStream(input, context, {
- push(content) {
+ push(content: string | null) {
if (content != null) {
if (!stream.push(content)) {
return new Promise<void>(resolve => {
resolveRead = resolve
})
}
} else {
stream.push(null)
}
},
- destroy(err) {
+ destroy(err: any) {
stream.destroy(err)
},
- } as any)
+ })📝 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.
| renderToSimpleStream(input, context, { | |
| push(content) { | |
| if (content != null) { | |
| if (!stream.push(content)) { | |
| return new Promise<void>(resolve => { | |
| resolveRead = resolve | |
| }) | |
| } | |
| } else { | |
| stream.push(null) | |
| } | |
| }, | |
| destroy(err) { | |
| stream.destroy(err) | |
| }, | |
| } as any) | |
| renderToSimpleStream(input, context, { | |
| push(content: string | null) { | |
| if (content != null) { | |
| if (!stream.push(content)) { | |
| return new Promise<void>(resolve => { | |
| resolveRead = resolve | |
| }) | |
| } | |
| } else { | |
| stream.push(null) | |
| } | |
| }, | |
| destroy(err: any) { | |
| stream.destroy(err) | |
| }, | |
| }) |
🧰 Tools
🪛 GitHub Check: test / lint-and-test-dts
[failure] 161-161:
Parameter 'err' implicitly has an 'any' type.
[failure] 150-150:
Parameter 'content' implicitly has an 'any' type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server-renderer/src/renderToStream.ts` around lines 149 - 164, The
inline callbacks passed to renderToSimpleStream are missing parameter type
annotations causing implicit any; update the push and destroy signatures to
match SimpleReadable: change push(content) to push(content: string | null) and
destroy(err: any) to destroy(err: any), remove the trailing "as any" cast on the
options object, and ensure resolveRead remains a Promise<void> resolver so the
returned Promise type stays correct (references: renderToSimpleStream, push,
destroy, stream, resolveRead in renderToStream.ts).
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests