losos/html: skip patch shortcut when cached DOM was wiped (#15)#16
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a stale-cache bug in the lightweight html.js renderer where rerendering the same tagged-template after external DOM clearing could hit the patch fast-path and silently update detached/orphan nodes (issue #15).
Changes:
- Extend the render cache hit condition to ensure the cached DOM is still attached to the container before using the patch shortcut.
- Update the inline comment to document the new stale-cache guard.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Same template — patch holes. Skip if cached DOM was wiped (#15). | ||
| if (prev && prev.strings === template.strings && prev.parts[0]?.node && container.contains(prev.parts[0].node)) { |
| // Same template — patch holes. Skip if cached DOM was wiped (#15). | ||
| if (prev && prev.strings === template.strings && prev.parts[0]?.node && container.contains(prev.parts[0].node)) { |
Tagged-template `strings` arrays are interned by the engine, so the
cache's `prev.strings === template.strings` matches forever — even
when the container's children were cleared externally between
renders. patch() then mutates detached nodes in `prev.parts` and
silently no-ops, leaving the container blank.
Add a `container.firstChild` guard before taking the patch
shortcut. Falls through to the existing fresh-build path when the
cache is stale. Using `container.firstChild` rather than checking a
specific part's node also keeps the fast-path live for static
templates with zero `${}` holes (empty parts array).
Patched in `losos/html.js`, the file actually exported by
package.json (`exports["./html"] → ./losos/html.js`); the
duplicate copy at the repo root is unpublished and is its own
problem (separate issue).
melvincarvalho
force-pushed
the
fix-issue-15-stale-parts-cache
branch
from
20547f7 to
92db250
Compare
melvincarvalho
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Same template — patch holes. Skip if cached DOM was wiped (#15). | ||
| if (prev && prev.strings === template.strings && container.firstChild) { |
There was a problem hiding this comment.
Declining the anchor approach. The foreign-DOM case requires a consumer to interleave render() with direct container.appendChild(foreign) between renders on the same container — an unusual pattern; either you manage that container with render() or you do direct DOM ops, not both. The actual #15 consumer (the LOSOS shell) does content.innerHTML = '' immediately before each pane.render(...), so when our render() runs, firstChild is null and the check correctly falls through. Keeping container.firstChild for the lean ethic — +0 cache fields, +0 lines on the build path.
linkedobjects/losos#16 added a `container.firstChild` guard to the patch fast-path in `render()`, so a stale cache after the container was wiped externally falls through to a fresh build instead of silently no-op'ing on detached nodes. Mirror the upstream change into the vendored copy at `losos/html.js`. Net diff: 2 lines, +90 bytes. Fixes source-pane going blank after tab-switching in the LOSOS shell.
Closes #15.
Summary
stringsarrays are interned, soprev.strings === template.stringsmatches the cache forever — even when the container was wiped externally between renders.patch()then mutates orphan nodes inprev.partsand silently no-ops.container.firstChildguard before taking the patch shortcut. Falls through to the existing fresh-build path on cache miss / stale cache.2 lines changed (one comment, one extra condition on the existing
if). +90 bytes.Why
container.firstChildand notcontainer.contains(prev.parts[0].node)The earlier draft of this PR used
prev.parts[0]?.nodefor the validity check. As Copilot pointed out, that breaks the patch fast-path for static templates with zero${}holes —partsis empty, the check fails, every rerender does a full rebuild and resets element state (input focus, selection, etc).container.firstChildworks for both shapes: it's truthy when the container has any rendered DOM at all, falsy afterinnerHTML = ''. No regression on static templates.Why only
losos/html.jsand not the roothtml.jspackage.jsonexports./html→./losos/html.js, andfilesonly includeslosos/. The duplicatehtml.jsat the repo root isn't published and isn't what consumers import. Fixing it doesn't reach anyone. Filed separately as #17 — eliminating that duplicate is a different problem.Test plan
node -c losos/html.jssyntax passes.nosdav/browser's clone — switching tabs no longer blanks source-pane (the originating real-world hit).render, clearinnerHTML,renderagain with same template) returns the expected DOM after the fix.html\static
`) rerendered twice still hits the patch fast-path (parts is empty butcontainer.firstChild` is truthy → no full rebuild).