fix: a directive inside an asyncAppend chunk escapes the error boundary - #1285
Conversation
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.
|
Review trail, carried over from #1282 This branch was reviewed on #1282 before GitHub closed that PR when its base branch was deleted. The content is identical, only the commit ids moved: The routing and the two try spans were confirmed correct. Everything found was a claim around them that did not match what runs. 1. Both doc surfaces said a nested Fixed in 2. A Fixed in 3. The rethrow comment was wrong on both of its claims. It said the part has no owner, but after this change it does ( Fixed in 4. One lowercase brand in prose (invariant 11), which the hook regex misses because of the trailing possessive. Fixed in the same commit. What is not covered: the reviewer read the diff BEFORE those fixes. |
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Read the whole diff against the branch state, including the fix commit the earlier round never saw. Nothing to change.
The parts I wanted to be sure of, since this PR is mostly about claims matching behaviour: the owner stamp sits above the same-iterable short-circuit and survives the later teardownChild(part), because neither that nor clearStaleDirectiveState touches __commitOwner. renderToNodes inside commitOutOfBand but outside commitInto keeps the pre-existing ordering, and commitInto(null, fn) degrades to fn(), so a detached marker cannot crash the new path. Widening the currentRenderRoot window over renderToNodes is inert beyond its purpose, since boundaryOwnerOf is its only reader and is idempotent on a resolved host.
The audit in the body checks out: renderToNodes has exactly two callers, and every unwrapped applyChildInner caller is render-reachable. The doc corrections are accurate, including the one that matters most, that a nested throw does NOT stop the stream, which holds even when the component overrides renderError(), because clearInstance does not tear down child parts and so a boundary re-render leaves the stream state intact.
No third doc surface was left stale, and the new prose does not trip invariant 11.
1 participant
Closes #1251
Targets
main. #1274 (issue #1268) and #1284 (issue #1250) are both merged; this was stacked behind them only because all three edit the same two doc paragraphs, not for any correctness dependency (different function, different failure mode).Replaces #1282, which GitHub closed when its base branch was deleted on the merge below it. Same branch, same work, no content difference; the review round and its resolutions live on #1282.
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.041cb9a1and7c099717.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.