diff --git a/.agents/skills/webjs/references/components.md b/.agents/skills/webjs/references/components.md index f4b02559f..7df1f4453 100644 --- a/.agents/skills/webjs/references/components.md +++ b/.agents/skills/webjs/references/components.md @@ -183,6 +183,8 @@ The boundary also covers `watch(signal)` (its notify microtask) and `until()` (i **A commit that throws leaves the directive's own state consistent, so the NEXT valid render is correct.** (One reconciler is still exempt: a plain `.map()` array whose item TEMPLATE SHAPE changes in the same render that throws can strand a row. `repeat()` is the keyed path and is repaired.) This matters because the corruption is otherwise silent: the renders that expose it are fully valid and log nothing after the first throw. The hole whose commit threw is marked so the next render re-applies it rather than skipping it as unchanged (its recorded value is never advanced past a throw, and would otherwise match exactly what the recovering render supplies, leaving a child region blank for good). `repeat()` additionally repairs its key map so the map describes the DOM again, and the next render is an ordinary reconcile that repositions every row (the failure was a permanently duplicated row); it is deliberately NOT a rebuild of the region, which would discard the node identity keyed reconciliation exists to preserve. `guard()` records its new deps only once the commit succeeds, so a later render with those same deps re-renders the region instead of short-circuiting past a region the throw had blanked; `until()` advances its resolved priority only after the commit succeeds, so a failed high-priority resolution does not refuse the lower-priority one behind it. +**Teardown is total as well.** Removing a row is not a commit and has no retry, so a throw while tearing one down cannot be allowed to abandon the rest. Unbinding a `ref` during teardown can never abort the removal of the remaining rows, and `repeat()` drops each leftover key from its map before touching that row, so the map never describes a row that has already been removed (which used to leave the row the app DELETED on screen, reorder the survivors, and let a later render that re-added that key reinsert the disposed instance). To make that hold, a `ref` whose object `value` setter throws is now SWALLOWED on teardown, matching the ref CALLBACK, which was already swallowed everywhere. That is a deliberate divergence from lit, which guards neither and propagates from both. It applies to teardown only: on the COMMIT path a throwing object-ref setter still reaches `renderError()`, because there the boundary can report it and the next render can repair it. + Decision rules. Use `async render()` for request-time server data that should be in the first paint (the default). Add `renderFallback()` when a client re-fetch's stale content would mislead. Use `Task` / signals for genuinely client-only data (a click, viewport, live updates). For SLOW data where blocking the first byte hurts, wrap the region in `` to stream it (the only way to show a first-paint fallback; see `client-router-and-streaming.md`). Do NOT fetch in `connectedCallback` for data knowable server-side, and do NOT prop-drill what a leaf can fetch itself. ## Task: client-only async data diff --git a/packages/core/src/render-client.js b/packages/core/src/render-client.js index b311dbd3c..7b54427ac 100644 --- a/packages/core/src/render-client.js +++ b/packages/core/src/render-client.js @@ -1078,12 +1078,19 @@ function clearInstance(inst, container) { for (const p of inst.bound) { if (p.kind === 'event') p.el.removeEventListener(p.name, p.dispatcher); if (p.kind === 'element') { + // Guarded for the same reason as the sibling in `disposeInstance`, and + // the stakes are higher here: this is the container-level teardown + // `render()` runs before a template swap, so a throw skips the rest of + // this loop AND the `replaceChildren()` below, leaving the old DOM in + // place with `host[INSTANCE]` never reassigned. Since `lastTarget` is + // cleared only after the write, every later swap of that container + // then throws at the same part, permanently. const prev = /** @type any */ (p).lastTarget; if (prev) { if (typeof prev === 'function') { try { prev(undefined); } catch { /* swallow */ } } else if (typeof prev === 'object') { - prev.value = undefined; + try { prev.value = undefined; } catch { /* swallow */ } } /** @type any */ (p).lastTarget = undefined; /** @type any */ (p).__lastEl = undefined; @@ -1756,12 +1763,33 @@ function disposeInstance(inst) { // Unbind any active ref so the user observes the element being // removed (callback receives undefined / Ref.value cleared). // Mirrors lit-html's cleanup-on-disconnect for element parts. + // + // BOTH branches swallow, and lit is not the reason: lit's ref directive + // guards neither, so a throw there propagates. The reason is that a + // teardown has to be TOTAL. `lastTarget` is cleared only AFTER these + // writes, so a throw leaves the part still pointing at the ref and + // every later teardown of the same instance throws at the same line + // forever. It also aborts the rest of this loop, so the remaining + // parts keep their listeners and their refs bound. A teardown has no + // retry either (a commit has the COMMIT_FAILED sentinel and a next + // render; this does not), so there is nothing a propagated error could + // usefully repair. + // + // The object branch is the one this adds. The callback branch was + // already guarded here AND on the commit path (`applyElement` wraps + // every `nextTarget(...)` / `prevTarget(undefined)` call), so a + // throwing ref CALLBACK has always been swallowed everywhere. What was + // inconsistent is the object ref, guarded on neither. This makes the + // two agree on TEARDOWN, which is where the totality argument bites. + // It does NOT touch the commit path, so `applyElement`'s object-ref + // writes still propagate to the component boundary, which has a route + // for the error and a next render to repair it. const prev = /** @type any */ (p).lastTarget; if (prev) { if (typeof prev === 'function') { try { prev(undefined); } catch { /* swallow */ } } else if (typeof prev === 'object') { - prev.value = undefined; + try { prev.value = undefined; } catch { /* swallow */ } } /** @type any */ (p).lastTarget = undefined; /** @type any */ (p).__lastEl = undefined; @@ -1828,9 +1856,15 @@ function reconcileRepeat(part, value) { state.map.delete(key); } else { if (existing) { + // Unmapped BEFORE the row is touched, for the reason spelled out + // in the leftover loop below: a key kept across a refused removal + // points at a half-removed row, and reusing that row later walks + // the removal off the end of the region. Same ordering, same + // trade, and the two have to agree or the invariant the catch + // relies on holds on one branch and not the other. + state.map.delete(key); disposeInstance(existing); removeBetween(existing.startNode, existing.endNode); - state.map.delete(key); } const { inst, frag } = buildDetached(/** @type any */ (tr)); parent.insertBefore(frag, marker); @@ -1838,10 +1872,20 @@ function reconcileRepeat(part, value) { } } - // Remove any keys that remain in the old map. - for (const inst of state.map.values()) { - disposeInstance(inst); - removeBetween(inst.startNode, inst.endNode); + // Remove any keys that remain in the old map. The key leaves the map + // BEFORE its row is touched and the removal is in a `finally`, so at any + // throw point `state.map` holds exactly the leftovers this pass has not + // reached, and a row whose dispose threw still leaves the document. + // Iterating a snapshot keeps the delete obviously safe rather than + // relying on the reader knowing that deleting during a Map iteration is + // legal. + for (const [k, inst] of [...state.map]) { + state.map.delete(k); + try { + disposeInstance(inst); + } finally { + removeBetween(inst.startNode, inst.endNode); + } } state.map = newMap; } catch (err) { @@ -1861,6 +1905,33 @@ function reconcileRepeat(part, value) { // reconcile against a truthful map, which repositions every row and // re-applies whatever the throw skipped. // + // That claim covers the REMOVAL loop as well as the walk, and only + // because the loop was written to earn it. It drops each key before + // touching that row and removes the nodes in a `finally`, so a throw + // mid-removal cannot merge `newMap` over a `state.map` still holding + // disposed, detached rows. That was the failure: the row the app DELETED + // stayed on screen, the survivors reordered, and a later render that + // re-added that key reinserted the detached instance. The invariant, at + // any throw point on either branch: every instance this pass has not + // destructively touched is described by exactly one of the two maps, + // `newMap` for the processed new keys and `state.map` for the leftovers + // not reached yet, which is what makes the merge below correct. The + // exception is the row named in the residual just below, whose removal + // refused part-way; that one is in neither map, by choice. + // + // The residual is a throw from `removeBetween` ITSELF, which only calls + // `removeChild` on nodes the renderer owns, so it takes a throwing DOM to + // reach. That row is already unmapped, so its remaining nodes stay in the + // document tracked by nothing and a later re-add of that key builds a + // second row beside them. Unmapping AFTER the removal instead would keep + // that key, and it is measurably worse rather than better: the row is + // half removed, its start marker gone and its end marker still in place, + // so the re-add hits the reuse branch and `moveRange` re-attaches the + // lone start marker AFTER the end marker. The next removal of that key + // then walks forward from a start that never reaches its end, taking the + // repeat part's own marker and every following sibling with it, and the + // region is dead for good. One untracked row beats a destroyed list. + // // Deliberately NOT a teardown-and-rebuild of the region. Rebuilding is // the obvious defensive move and it is measurably worse: it discards node // identity for every row, which is the exact cost keyed reconciliation @@ -1884,9 +1955,16 @@ function reconcileRepeat(part, value) { /** @param {{ kind: 'repeat', map: Map }} state */ function teardownRepeat(state) { - for (const inst of state.map.values()) { - disposeInstance(inst); - removeBetween(inst.startNode, inst.endNode); + // Same delete-as-you-go shape as the leftover loop in `reconcileRepeat`, + // for the same reason: a throw part-way must not leave already-removed + // instances in the map. The trailing `clear()` stays as a no-op safety net. + for (const [k, inst] of [...state.map]) { + state.map.delete(k); + try { + disposeInstance(inst); + } finally { + removeBetween(inst.startNode, inst.endNode); + } } state.map.clear(); } diff --git a/packages/core/test/rendering/browser/directive-commit-throw.test.js b/packages/core/test/rendering/browser/directive-commit-throw.test.js index a2e7301c4..6f553a3d9 100644 --- a/packages/core/test/rendering/browser/directive-commit-throw.test.js +++ b/packages/core/test/rendering/browser/directive-commit-throw.test.js @@ -16,7 +16,7 @@ import { html } from '../../../src/html.js'; import { render } from '../../../src/render-client.js'; import { repeat } from '../../../src/repeat.js'; -import { watch } from '../../../src/directives.js'; +import { watch, ref } from '../../../src/directives.js'; import { signal } from '../../../src/signal.js'; import { WebComponent } from '../../../src/component.js'; @@ -215,6 +215,77 @@ suite('directive commit throws (browser)', () => { assert.strictEqual(after[2], before[2]); }); + test('a throwing ref unbind removes the row, and re-adding it builds a new element', () => { + // The identity facts linkedom cannot prove: the survivor is MOVED rather + // than rebuilt, and the resurrected key is a genuinely new element rather + // than the disposed instance handed back. + const boom = { set value(v) { if (v === undefined) throw new Error('ref-boom'); }, get value() { return null; } }; + const refRows = (items) => html``; + + render(refRows([{ id: 1, n: 'a' }, { id: 9, n: 'doomed' }]), container); + const before = [...container.querySelectorAll('li')]; + + render(refRows([{ id: 1, n: 'a' }]), container); + assert.deepEqual([...container.querySelectorAll('li')].map((li) => li.textContent), ['a']); + assert.strictEqual(container.querySelector('li'), before[0]); + + render(refRows([{ id: 1, n: 'a' }, { id: 9, n: 'again' }]), container); + const after = [...container.querySelectorAll('li')]; + assert.deepEqual(after.map((li) => li.textContent), ['a', 'again']); + assert.strictEqual(after[0], before[0]); + assert.ok(after[1] !== before[1], 'the disposed instance must not be resurrected'); + }); + + test('a refused DOM removal keeps that row keyed, and does not duplicate it', () => { + // The ref-unbind case above cannot reach the removal loop's own shape, + // because the guard makes that step unable to throw at all. This drives + // the throw from the DOM removal instead, in a real browser, where node + // identity is the thing that separates "reused the row already there" + // from "built a second one beside it". + const idRows = (items) => html``; + + render(idRows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 3, n: 'three' }]), container); + const [liOne, liTwo, liThree] = [...container.querySelectorAll('li')]; + + const ul = container.querySelector('ul'); + const origRemove = ul.removeChild.bind(ul); + ul.removeChild = (node) => { + if (node === liThree) throw new Error('rm-boom'); + return origRemove(node); + }; + throwsMatching(() => { render(idRows([{ id: 1, n: 'one' }]), container); }, /rm-boom/); + ul.removeChild = origRemove; + + render(idRows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }]), container); + const after = [...container.querySelectorAll('li')]; + + // Key 1 never left the map, so it is the same element. Key 2 left the map + // together with its row, so it MISSES and rebuilds rather than having a + // disposed instance handed back. + assert.strictEqual(after.filter((li) => li === liOne).length, 1); + assert.ok(!after.includes(liTwo), 'a removed row must not be resurrected'); + assert.strictEqual(after.filter((li) => li.textContent === 'two').length, 1); + + // `liThree` is the named residual: the DOM removal itself refused, so + // those nodes stayed, and its key was already dropped, so nothing tracks + // them. What the trade buys is that the region still RECONCILES, which is + // the assertion that matters and the one only a real browser settles. + assert.strictEqual(liThree.parentNode, ul); + render(idRows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 4, n: 'four' }]), container); + assert.deepEqual( + [...container.querySelectorAll('li')].map((li) => li.textContent).filter((t) => t !== 'three'), + ['one', 'two', 'four'], + ); + }); + test('removing rows after recovery leaves nothing behind', () => { render(rows(good), container); throwsMatching(() => { diff --git a/packages/core/test/rendering/directive-commit-throw.test.js b/packages/core/test/rendering/directive-commit-throw.test.js index 546c6b2d3..409c34df9 100644 --- a/packages/core/test/rendering/directive-commit-throw.test.js +++ b/packages/core/test/rendering/directive-commit-throw.test.js @@ -27,11 +27,11 @@ before(() => { globalThis.HTMLElement = window.HTMLElement; }); -let html, render, guard, until, watch, repeat, signal; +let html, render, guard, until, watch, ref, repeat, signal; before(async () => { ({ html } = await import('../../src/html.js')); ({ render } = await import('../../src/render-client.js')); - ({ guard, until, watch } = await import('../../src/directives.js')); + ({ guard, until, watch, ref } = await import('../../src/directives.js')); ({ repeat } = await import('../../src/repeat.js')); ({ signal } = await import('../../src/signal.js')); }); @@ -39,6 +39,18 @@ before(async () => { /** A value that throws when a commit stringifies it. */ const poison = { toString() { throw new Error('boom'); } }; +/** + * A ref whose object write throws on UNBIND. Every other poison in this file + * throws from a COMMIT, and none of them can reach the teardown paths below: + * a commit stringifies its value on the way into the DOM, while these throws + * come from tearing a row back out, which is a different code path with a + * different repair. Only an object ref (or a callback ref) is called during + * teardown at all, so it is the only way in. + */ +function throwingRef(message) { + return { set value(v) { if (v === undefined) throw new Error(message); }, get value() { return null; } }; +} + /** Text content of the rendered rows, ignoring marker comments. */ function rowTexts(container) { return [...container.querySelectorAll('li')].map((li) => li.textContent); @@ -167,6 +179,132 @@ test('repeat: a throw in a nested-template row recovers', () => { assert.deepEqual([...container.querySelectorAll('b')].map((b) => b.textContent), ['one', 'two']); }); +// --- teardown throws (repeat's leftover-removal loop, and clearInstance) --- + +test('repeat: dropping a row whose ref unbind throws still removes that row', () => { + const container = document.createElement('div'); + const rows = (items) => html``; + + render(rows([{ id: 1, n: 'a' }, { id: 9, n: 'doomed' }]), container); + assert.deepEqual(rowTexts(container), ['a', 'doomed']); + const beforeA = container.querySelector('li'); + + // The app asked for a one-row list. It used to get a two-row list led by + // the row it deleted, because the unbind threw out of the removal loop. + render(rows([{ id: 1, n: 'a' }]), container); + assert.deepEqual(rowTexts(container), ['a']); + + // And the survivor is the same element, not a rebuild. + assert.equal(container.querySelector('li'), beforeA); + + // Still reconciling normally afterwards, rather than wedged. + render(rows([{ id: 1, n: 'A' }]), container); + assert.deepEqual(rowTexts(container), ['A']); +}); + +test('repeat: re-adding a key dropped through a throwing unbind builds a fresh row', () => { + const container = document.createElement('div'); + const rows = (items) => html``; + + render(rows([{ id: 1, n: 'a' }, { id: 9, n: 'doomed' }]), container); + const doomed = [...container.querySelectorAll('li')][1]; + + render(rows([{ id: 1, n: 'a' }]), container); + render(rows([{ id: 1, n: 'a' }, { id: 9, n: 'again' }]), container); + + assert.deepEqual(rowTexts(container), ['a', 'again']); + // A disposed, detached instance must never come back: it is unmapped, so + // the key misses and builds fresh. + const readded = [...container.querySelectorAll('li')][1]; + assert.notEqual(readded, doomed); +}); + +test('repeat: a throw INSIDE the removal loop leaves no leftover still mapped', () => { + // Drives the throw from the loop's OTHER step, so this covers the loop's + // shape independently of the ref guard above (which makes the dispose step + // unable to throw at all). Nothing here reaches into module internals: it + // patches the rows' parent so one DOM removal refuses. + const container = document.createElement('div'); + const rows = (items) => html``; + + render(rows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 3, n: 'three' }]), container); + const [, liTwo, liThree] = [...container.querySelectorAll('li')]; + + const ul = container.querySelector('ul'); + const origRemove = ul.removeChild.bind(ul); + ul.removeChild = (node) => { + if (node === liThree) throw new Error('rm-boom'); + return origRemove(node); + }; + + // Drop keys 2 and 3. Key 2 is processed cleanly; key 3's DOM removal + // refuses part-way. + assert.throws(() => { render(rows([{ id: 1, n: 'one' }]), container); }, /rm-boom/); + ul.removeChild = origRemove; + + // `liThree` is the named residual: `removeBetween` itself refused, so those + // nodes stayed, and the key was already dropped, so nothing tracks them. + assert.equal(liThree.parentNode, ul); + + // Re-add BOTH dropped keys in ONE render, with no render in between. That + // ordering is load-bearing rather than incidental: any render that treats + // key 3 as a leftover again unmaps it under EITHER unmap ordering (its + // start marker is gone, so `removeBetween` early-returns and the delete + // runs), which collapses the difference this test exists to catch. Re-added + // immediately, key 3 is already unmapped, so it MISSES and builds a fresh + // row beside the remnant. Unmapping after the removal instead would keep + // that key pointing at a half-removed row, the re-add would take the reuse + // branch, and `moveRange` would re-attach the lone start marker AFTER its + // own end marker. + render(rows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 3, n: 'three' }]), container); + const at = (text) => [...container.querySelectorAll('li')].filter((li) => li.textContent === text); + + assert.equal(at('one').length, 1); + assert.equal(at('two').length, 1, 'exactly one row for the re-added key'); + assert.notEqual(at('two')[0], liTwo, 'a disposed instance must not be resurrected'); + assert.equal(at('three').length, 2, 'a fresh row for the re-added key, beside the remnant'); + + // And the region is still alive. Under the other ordering the removal below + // walks off the end of the mis-ordered range and takes the repeat part's + // own marker with it, after which no render ever lands again. + render(rows([{ id: 1, n: 'one' }, { id: 2, n: 'two' }, { id: 4, n: 'four' }]), container); + assert.deepEqual( + [...container.querySelectorAll('li')].map((li) => li.textContent).filter((t) => t !== 'three'), + ['one', 'two', 'four'], + 'the region still reconciles rather than being dead', + ); +}); + +test('clearInstance: a throwing ref unbind does not wedge template swaps', () => { + const container = document.createElement('div'); + render(html`

A

`, container); + assert.equal(container.querySelector('p')?.textContent, 'A'); + + // A template-SHAPE swap runs the container-level teardown. The throw used + // to skip the replaceChildren() at the end of it, so the old DOM stayed and + // the instance was never replaced. `lastTarget` is cleared only after the + // throwing write, so it was permanent: every later swap threw at the same + // part, which is why this asserts TWO swaps. + render(html`B`, container); + assert.equal(container.querySelector('b')?.textContent, 'B'); + assert.equal(container.querySelector('p'), null); + + render(html`C`, container); + assert.equal(container.querySelector('i')?.textContent, 'C'); +}); + test('a plain template child hole recovers too (not just repeat)', () => { const container = document.createElement('div'); const view = (x) => html`
${x}
`; diff --git a/website/app/docs/error-handling/page.ts b/website/app/docs/error-handling/page.ts index 4f9dad74b..d678389a9 100644 --- a/website/app/docs/error-handling/page.ts +++ b/website/app/docs/error-handling/page.ts @@ -128,6 +128,7 @@ export default function GlobalError({ error }: { error: Error }) {

A directive that throws mid-commit stays consistent

The component boundary above also covers watch(signal) and until(), which commit outside the update cycle, so a throw from either reaches renderError() rather than the window. It reaches the component whose template holds the binding, which is not always the element the binding sits inside: a watch() written between a child component's tags belongs to the parent that wrote it. asyncAppend / asyncReplace are not covered, in two ways: that path logs its own iteration throw and continues on purpose, and a watch() or until() nested inside a chunk it commits still reaches the window.

Beyond reporting the error, the directive's own state is left describing the DOM that actually exists, which is what makes the NEXT render correct. That matters because the failure is otherwise silent: the renders that expose it are fully valid and log nothing. The hole whose commit threw is marked so the next render re-applies it instead of skipping it as unchanged, which is what used to leave a region blank for good. repeat() additionally repairs its key map so it describes the DOM again, and the next render is an ordinary reconcile that repositions every row (the symptom was a permanently duplicated row); it deliberately does not rebuild the region, which would throw away the node identity keyed reconciliation exists to preserve. guard() records its new deps only once the commit succeeds, so a later render with those deps re-renders the region instead of skipping past one the throw had blanked; until() advances its resolved priority only after its commit succeeds, so a failed high-priority resolution does not refuse the lower-priority one behind it.

+

Tearing content back out is covered too, and it has to be, because a teardown has no next render to repair it. Unbinding a ref while a row is removed can never abort the removal of the rest of the list, and repeat() drops each leftover key from its map before touching that row, so the map never describes a row that has already been removed. Without that, a throw part-way through left the row you had DELETED on screen, reordered the survivors, and let a later render that re-added the key reinsert the disposed instance. The cost is that a ref whose object value setter throws is swallowed on teardown, matching the ref callback, which was already swallowed everywhere (lit guards neither and propagates from both, so this is a deliberate divergence). It applies to teardown only: on the COMMIT path a throwing object-ref setter still reaches renderError().

Server action errors

Errors thrown from server actions are sanitized in production: the client gets a generic "Internal server error" message plus a short digest, never the raw thrown message or the stack trace. The full error is logged server-side keyed by that digest, so a client-reported digest maps back to the server log line. A redirect() / notFound() control-flow throw passes through. To surface a specific user-facing message, return an ActionResult { success: false, error } envelope instead of throwing.