Document the terminated marker contract and pin it with tests - #750
Open
titouanmathis wants to merge 2 commits into
Open
Document the terminated marker contract and pin it with tests#750titouanmathis wants to merge 2 commits into
titouanmathis wants to merge 2 commits into
Conversation
Export SizeUnchanged@studiometa/js-toolkit
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #750 +/- ##
=======================================
Coverage 97.17% 97.17%
=======================================
Files 170 170
Lines 4143 4143
Branches 1151 1152 +1
=======================================
Hits 4026 4026
Misses 106 106
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
titouanmathis
force-pushed
the
fix/mutation-scan-remount-and-swap-order
branch
from
August 1, 2026 18:51
ca96f1c to
ee95feb
Compare
titouanmathis
changed the title
titouanmathis
changed the title
Code ReviewRisk: Low — The change documents the existing terminated-marker contract and adds regression coverage without changing runtime behavior; it is safe to merge. The MR explains how automatic mount scanning handles terminated elements, including disconnect-triggered termination and the explicit remount escape hatch. It also adds focused tests covering same-node reinsertion, a positive mount control, replacement nodes, and explicit instantiation. Review usage: 39,350 in (31,724 cached) / 865 out tokens — $0.0089 (openrouter/openai/gpt-5.6-luna, thinking: low) Reviewed by @weareikko/code-review v0.9.5 for commit 99eb6c4. Previous review runsPrevious run archived 2026-08-16T19:57:05ZCode ReviewRisk: Low — The change adds documentation and regression tests for the existing terminated-marker and automatic mount-scan contract, with no production behavior changes. It documents how disconnect-driven termination prevents automatic remounting of the same DOM node and records the supported escape hatches. The tests cover both permanent skipping during automatic scans and explicit remounting through No issues found. Review usage: 8,931 in / 350 out tokens — $0.0066 (openrouter/openai/gpt-5.6-luna, thinking: low) Reviewed by @weareikko/code-review v0.9.5 for commit 20b44b3. |
Document and pin the existing contract: `$terminate()` is one-shot per DOM element by design. A terminated element keeps its `'terminated'` marker, so the document-wide mount scan skips it forever — re-inserting the same node does not remount it. Replacing the element with a brand new node is the supported way to mount a fresh instance. This backs lazy/once components such as `withMountWhenInView`, which must trigger exactly once when their element first enters the viewport and must never re-trigger on the same element. No behavior change: adds a doc comment on `$terminate()` and the mount scan site, plus a regression test asserting both halves of the contract (same-node re-insertion stays terminated; replacement remounts). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NeZwHwo3d9rYsCxJUQqTep
The previous commit recorded two claims that do not hold. 1. It said the `'terminated'` marker "backs lazy/once components such as `withMountWhenInView`". It does not: `withMountWhenInView` calls `$mount()` on every enter and `$destroy()` on every leave, so it re-triggers on the same element by design and never calls `$terminate()`. Replaced with the rationale git history supports — commit `9bc6f838` changed `__base__.delete()` to `__base__.set(..., 'terminated')` so the marker survives and keeps the element excluded from `$children`, `$parent` and `queryComponent()` resolution, which already filtered on the `'terminated'` string. 2. It said "the only supported way to mount again is to replace the element with a brand new node". The constructor overwrites `__base__` unconditionally, so `new Ctor(el).$mount()` clears the marker and mounts a terminated node. The claim is now scoped to the automatic document-wide scan, with both escape hatches named. Also record who actually calls `$terminate()`: the terminate pass of `mutationCallback()` terminates any instance whose element is disconnected, so users reach this state without calling `$terminate()` themselves. The marker (2022) and that pass (2024, issue #552) were added separately, so the combination is framed as a sharp edge with stated workarounds, not as design intent. Adds the public docs paragraph the contract was missing under `$terminate()`, and hardens the test: a positive control element proves the mount scan really ran during the re-insertion batch, a second test pins the `new Ctor(el).$mount()` escape hatch, and both live in their own `describe` block instead of the `addToRegistry` one. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0114BLVFkrWWJAhmamS6NHB3
titouanmathis
force-pushed
the
fix/mutation-scan-remount-and-swap-order
branch
from
August 16, 2026 19:56
20b44b3 to
99eb6c4
Compare
Export sizeBundled per export with peer dependencies left external, dynamic imports excluded and the output minified; sizes are gzipped. ✅ No export size changes. Unchanged (388)@studiometa/js-toolkit
@studiometa/js-toolkit-v4
|
v3 mount benchmarksBase and head measured on this runner, alternating over 3 rounds each; every value is the median of the round medians. Running both sides on one machine is what removes cross-machine noise — a cached baseline from another runner would put it back. A move under 25%, or on a benchmark under 5 ms, is not reported as a change: it is inside the measured noise of a shared runner. No benchmark moved beyond the noise floor. Within noise (10)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Document and pin an unguarded contract in the mount scan, and correct two claims the first version of this branch got wrong.
Docs + tests only. No behavior change.
The contract
$terminate()stamps the element's__base__entry with a'terminated'marker instead of deleting it. That marker is truthy, sogetInstanceFromElement()returns it and the document-wide auto-mount scan (mutationCallbackinBase/utils.ts) treats the element as already having an instance and skips it — forever, including when the very same node is re-inserted into the DOM.Before this PR,
packages/js-toolkit/Base/utils.ts:194— the line that decides this — was guarded by no test at all. Mutating it to ignore the'terminated'marker broke exactly one test out of the whole suite.Why the marker exists
Commit
9bc6f838("Fix terminated feature", Feb. 2022) changed__base__.delete(...)to__base__.set(..., 'terminated'). The one-line change made an already existing filter work:ChildrenManagerwas already doing.filter((instance) => instance !== 'terminated'), but with the entry deleted,__getChild()fell through and constructed a fresh instance instead. The marker is what keeps terminated elements excluded from$children,$parentandqueryComponent()resolution.The sharp edge
You rarely call
$terminate()yourself. The second pass ofmutationCallback()terminates any instance whose element is no longer connected. So removing an element from the DOM is enough to permanently disable it for the auto-mount scan — animated modal teardown, virtual-list recycling, view transitions and drag & drop re-parenting all hit this without anyone typing$terminate().This combination is emergent, not designed: the marker landed in Feb. 2022 and the terminate-on-disconnect pass in Dec. 2024 (issue #552), for unrelated reasons. The docs frame it as a sharp edge with stated workarounds, not as intent.
Changes
packages/js-toolkit/Base/Base.ts— rewrote the$terminate()JSDoc: the real (git-supported) rationale, the mount-scan consequence scoped to the automatic scan, both escape hatches, and the disconnect scan named as the actual caller.packages/js-toolkit/Base/utils.ts— comment on the mount-scan site explaining why a truthy marker skips the element and what that does and does not imply.packages/docs/api/instance-methods.md— new### Removing an element terminates its componentsection under$terminate(): the auto-terminate behavior, the re-insertion no-op, and three workarounds (re-parent synchronously, insert a new node, instantiate explicitly), plus a tip pointing at$destroy()+$mount().packages/tests/Base/utils.spec.ts— two regression tests in their owndescribe('The terminated marker and the auto-mount scan')block:new Ctor(el).$mount()does mount a terminated node — pinning the escape hatch the docs now promise.Corrections vs. the first version of this branch
Two claims in the initial commit were wrong and are removed:
"it backs lazy/once components such as— the opposite is true.withMountWhenInView, which must trigger exactly once … and must never re-trigger on the same element"withMountWhenInViewcalls$mount()on enter and$destroy()on leave on every visibility flip, and never calls$terminate()(rg '\$terminate\(' packages/js-toolkit/hits only the definition and the disconnect scan). Its soleterminatedreference is a passive listener that disconnects the observer."The only supported way to mount again is to replace the element with a brand new node."—Base.ts:441-443overwrites__base__unconditionally, sonew Ctor(el).$mount()clears the marker and mounts. Verified by running it; now covered by test 2 above.The initial commit
ee95feb1still carries the first of these in its message — squash on merge so it does not land in history.Not addressed here
getInstances()—$on/$emituse$elas the EventTarget, so the old instance'sbefore-mountedlistener fires too. Pre-existing and separate; the test asserts on the instance, not on the count.Verification
packages/tests/Base/utils.spec.tsrun 5× in isolation: 16 passed each time, no flakiness.npm run lintandnpm run build: clean.🤖 Generated with Claude Code
https://claude.ai/code/session_0114BLVFkrWWJAhmamS6NHB3