fix: a directive inside an asyncAppend chunk escapes the error boundary - #1282
fix: a directive inside an asyncAppend chunk escapes the error boundary#1282vivek7405 wants to merge 2 commits into
Conversation
c2ac400 to
a324bc8
Compare
f9b7b7c to
4c47403
Compare
a324bc8 to
5ff87a5
Compare
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Routing is right and the two spans are the correct shape. Everything I found is a claim around it that does not match what runs.
Both doc surfaces bind the stream stopping to a nested watch / until throw as well as to the chunk commit. Only the commit sets aborted; a nested directive throws from its own handler, outside the loop entirely, so the loop keeps pulling. Easy to state and easy to test, so both now happen.
The mapper sits in the iteration span, so a mapper throw stays at the console, but the docs said the iterable was the only thing that did. The mapper is the code producing the very TemplateResult being committed, so that omission is the sort a reader would act on.
The rethrow comment was wrong twice over: the part DOES have an owner after this change (render() stamps the container itself), so the rethrow is the owner-has-no-boundary branch; and watch throws inside a queueMicrotask, which is an uncaught error, not a rejection. Also one lowercase brand in prose, which the hook regex misses because of the trailing possessive.
Where each one is, since the anchors moved under a rebase:
.agents/skills/webjs/references/components.mdandwebsite/app/docs/error-handling/page.ts, the async-stream sentence in the mid-commit paragraph: bound the stop to the nested case as well as the commit, and named the iterable as the only thing left at the console.packages/core/src/render-client.js, the rethrow comment insideconsumeAsyncStream's commit catch: claimed the part has no owner, and called the surfacing an unhandled rejection likewatch.packages/core/src/render-client.js,consumeAsyncStream's doc comment: lowercase brand in prose.
|
All fixed in Both doc surfaces now scope the stop to the chunk commit and say a nested directive reaches the boundary without stopping the loop, and both name the The rethrow comment now says the rethrow comes from an owner carrying no |
4c47403 to
04ef620
Compare
A watch() or until() nested inside a chunk committed by asyncAppend / asyncReplace never got an error-boundary owner, so a throw from its commit escaped to the window instead of reaching the owning component's renderError(). consumeAsyncStream is the third out-of-band commit site and was the one #1172 did not wrap: it commits with no render on the stack, so any directive installed by that commit saw no currentRenderRoot, was never stamped, and its later throw fell through to a bare rethrow in a microtask. Both directives now stamp the owner at install time, above the same-iterable short-circuit so a re-render that returns early still refreshes it, and the chunk commit runs inside commitOutOfBand with renderToNodes INSIDE the wrap, since that is where a nested directive reads the owner. The one catch also splits in two. A chunk COMMIT throw is a render failure of the component whose template holds the binding, so it routes to renderError() and stops the stream, because the boundary is about to render an error state and appending into a region it may have replaced is not a recovery. A throw from the author's own iterable keeps its console.error, which is a separate standing decision about the author's generator rather than about a render. The split has to be structural rather than a flag: reportOutOfBandCommitError rethrows for an ownerless part, so calling it inside the old outer try would hand that rethrow straight to the swallow this fixes.
…he author's Corrections to the claims around the async-stream routing, not to the routing. Both doc surfaces bound the stream stopping to a nested watch or until throw as well as to the chunk commit. Only the commit stops it: a nested directive throws from its own handler, outside the loop entirely, so the loop keeps pulling. There is now a test that lets the generator yield again after a nested throw and asserts exactly that. The mapper is the author's code, not a render, and it sits in the iteration span with the iterable, so a mapper throw stays at the console. The docs said the iterable was the only thing that did. Both now name the mapper, and a test pins it. The rethrow comment claimed the part has no owner and that the surfacing is an unhandled rejection like watch and until. The part does have an owner after this change (render() stamps the container itself), so the rethrow comes from that owner carrying no _handleRenderError; and watch throws inside a queueMicrotask, which is an uncaught error rather than a rejection. Both corrected, and the ownerless path is now stated in the docs. Also fixes the one prose brand casing in the file.
5ff87a5 to
e49fc69
Compare
1 participant
Closes #1251
Stacked on #1277 (which closes #1250, itself stacked on #1274 / #1268). Base is
fix/map-array-commit-throw, so review the top commit; GitHub retargets this as the stack merges. The stacking is for the doc paragraphs all three edit, not a correctness dependency: this is a different function and a different failure mode.A
watch()oruntil()nested inside a chunk committed byasyncAppend/asyncReplacenever got an error-boundary owner, so a throw from ITS commit escaped to the window as an uncaught error instead of reaching the owning component'srenderError(). #1172 routed out-of-band commit throws by stamping the owner at directive-install time and wrappingwatch's notify microtask anduntil's promise handler.consumeAsyncStreamis the third such site and was not wrapped: it commits throughrenderToNodeswith no render on the stack, so any directive installed by that commit sawcurrentRenderRoot === null, was never stamped, and its later throw fell through to a bare rethrow inside a microtask.This was NOT the documented carve-out. That carve-out is about the stream swallowing its own ITERATION errors. A nested directive's commit throw was neither swallowed nor routed, it simply escaped.
What changed
Both directives stamp the owner at install time, above the same-iterable short-circuit so a re-render that returns early still refreshes it, and guarded so a re-install outside a render keeps a previously good owner.
The chunk commit runs inside
commitOutOfBand, withrenderToNodesINSIDE the wrap, since that is where a nested directive is installed and where it readscurrentRenderRoot.commitIntois a different concern (the renderer-write window for a light slot host), so the two nest rather than replace each other.The one catch splits into two spans, which the filing left open and this settles. A chunk COMMIT throw is a render failure of the component whose template holds the binding, so it routes to
renderError()and STOPS the stream (the boundary is about to render an error state, and appending into a region it may have replaced is not a recovery). A throw from the author's own iterable keeps itsconsole.error, because that is the author's generator failing rather than a render.lit is no authority either way:
forAwaitOfhas no try at all andAsyncReplaceDirective.updateneither awaits nor catches it, so in lit BOTH failures become unhandled rejections at the window, and lit has no per-component boundary to route to. Webjs does, and per-component error isolation says a commit throw belongs to the owning component. Routing a nestedwatchthrow while leaving the identical throw from the chunk's own template at the console would have been an indefensible seam.The split is structural, not a flag, and that is load-bearing:
reportOutOfBandCommitErrorRETHROWS for a part with no owner, so calling it from inside the old outer try would hand that rethrow straight back to theconsole.errorswallow, which is the escape being fixed.Audit of the remaining out-of-band commit sites
Recorded so this closes the SET rather than one instance:
renderToNodesis called only fromconsumeAsyncStream(now wrapped) and recursively from itself.applyChildInner's unwrapped callers are all reached from a render, socurrentRenderRootis already set:applyChild,applyChildInnerRaw(thekeyedandguardpaths),applyCache,applyUntil's two synchronous calls, andapplyWatch's two synchronous calls.until's resolution andwatch's notify.Correction shipped alongside
The docs said this path "logs its own iteration throw and continues on purpose". It never continued: the catch sat OUTSIDE the
while, so an iteration throw has always ended the stream. Both doc surfaces now say so.Test plan
packages/core/test/rendering/directive-commit-throw.test.js): six new cases. A nestedwatchforasyncAppendand forasyncReplace; a nesteduntil(the two directives stamp independently); one through theownershipTesthelper, whoseowner-elholding achild-elis what distinguishes the correct owner from a lucky one; the stream's OWN chunk commit throw reaching the boundary, not being logged, and stopping the stream (asserted by the generator not being pulled again); and an ITERATION throw still logged and never routed. Every one asserts nothing escaped, viauncaughtExceptionandunhandledRejectionlisteners held only for the assertion window, since "the boundary was called" cannot distinguish routed from routed-and-also-escaped. 25/25 in the file, 402/402 acrosspackages/core/test/rendering.c2ac4005.packages/core/test/rendering/browser/directive-commit-throw.test.js): a nestedwatchagainst a REAL component in both light and shadow mode. Shadow is the case the unit tests cannot reach, since only there does the render root differ from the boundary-carrying element andboundaryOwnerOfhas to resolve aShadowRootthrough its.host. A windowerrorlistener asserts nothing escaped. Full run green on Chromium, Firefox and WebKit.render-client.jsis the client renderer, not a runtime-sensitive surface, and the parity hook does not match this path.Doc surfaces
.agents/skills/webjs/references/components.md: the sentence saying this path is not covered "in two distinct ways" now states what is true, including that an iteration throw ends the stream.website/app/docs/error-handling/page.ts: the same correction in that page's prose.