feat(v4): make the instance lookup say which population it answers - #865
Merged
Conversation
`getInstances()` filtered on `$isMounted` and said nothing about it at
the call site. `getInstances('Foo')` returning 2 where three elements
declare `Foo` — one below its `media:` breakpoint — was undebuggable
from the name, and the raw map read the filter hides had no public
spelling at all, so 52 spec files reached past `getInstances()` for a
private helper instead.
Four exports replace the one, over a single internal `collect()` plus a
predicate, so the traversal is written once:
- `getInstances()` — every instance built, mounted or not.
- `getMountedInstances()` — the live ones, what you may call a method on.
- `getUnmountedInstances()` — built, then stood down.
- `getInstance(el, name)` — one map read, no scan.
Dropping the `$isMounted` filter resurrects nothing. The string form
narrows three times, and the middle step already does the work the
filter is credited with: `selectorFor(name)` over-matches by design,
the `INSTANCES` read narrows to *constructed*, and `$isMounted` narrowed
to *live*. An inactive declaration has no instance, and a
breakpoint-withdrawn one is destroyed **and deleted from the map** by
`reconcileElement()`. So the doc comment on `selectorFor()` — which
attributed the narrowing to a mount check — is rewritten to name the map
read instead, with the reason not to re-add the filter, because the
filter would also hide the one population that is real: the instances a
reversible `in-view` or `media:` strategy has legitimately unmounted and
kept for the crossing back.
There is deliberately no `getMountedInstance`. The singular returns one
object; a caller who wants the live one reads `.$isMounted` on it,
rather than trusting an `undefined` that would mean two things.
Two asymmetries are now documented rather than discovered: the element
form works on a detached element and the string form cannot reach one
from `document`, and the string form is DOM order where the element form
is mount order.
Root barrel: 86 exports to 89. Subpath stubs regenerated with
`npm run subpaths`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
`getInstances()` no longer filters on `$isMounted`, so every existing call site had to be reread rather than renamed. Each of these picked the function that matches what the code does with the result, not the one that keeps the old behaviour by default. `getMountedInstances()`, because the result is *used*: - `ActionEvent.instances` builds the effect's name→instance map and the effect calls methods on it. - `ActionEvent.targets` resolves the components an event runs effects on. A stood-down `in-view` target must not be run against. - `Sticky.instances` stacks siblings by index and sums their heights. An unmounted `Sticky` has no position to contribute. - The `mount()`, `resetDom()`, `recordEvents()` and `resetRegistry()` specs on `src/test/index.spec.ts`, and the responsive-set spec, all assert "this component is live" — which `getInstances()` no longer says. `getInstances()`, kept, because the claim is "nothing was ever built": - `autoload.spec.ts` on a lazy declaration before its class arrives — the assertion is now stronger, since no filter can be hiding an instance. - The two post-`resetRegistry()` assertions, for the same reason. Worth recording: the suite was green both before and after this commit. No test distinguished the two populations at any of these sites, which is the argument for the rename — the filter was invisible to the callers and to their specs alike. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
`getInstance()` is core's now, so the specs that were reaching around the public surface for it stop doing that. `migration/**` takes it off the root barrel next to `registerComponents`, and `src/**` takes it off `instances.js` like every other core module it imports. Behaviour is identical — the same raw map read, no filter. The one real change is the type. The private helper was `(el: Element | null, name: string): T`, which lied twice: it accepted a `querySelector()` result and claimed the instance was always there. The public one is `(el: Element, name: string): T | undefined`, so a spec that knows the element and the instance exist now says so with `!`. That is the whole of the churn here, and it is worth its weight: the places where `!` had to go on the *argument* are exactly the places a `querySelector()` miss would have produced `undefined` from a helper whose return type said it could not. Where the spec was already handling the absence — `toBeUndefined()`, `toBeTruthy()`, `Boolean(...)`, a `waitFor()` predicate polling for the instance to appear — no assertion was added, because the point of the call there is that it may be `undefined`. `src/test-utils.js` still exports `countRequestedFrames`, so it survives this commit; four spec files still import it from there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
It was refused from the shipped test module for one caller and because "a global patch is a poor thing to hand out". It has four caller files now — `Cursor`, `smoothTo`, the service mixins and `drag` — and the question it answers is one no consumer can answer for itself: the framework's own scheduler owns the `requestAnimationFrame` calls, so there is no seam of the component's for a spy to sit on. "This did not schedule a frame per pointer event" is exactly the kind of thing the rest of this module exists for. The global patch is not hidden. The doc comment says outright that `globalThis.requestAnimationFrame` is replaced for the duration of the callback and restored in a `finally` — so it comes back whether the callback returns or throws — and it names the one way to misuse it: two concurrent calls would nest their wrappers. Three tests come with it: the count, the restore-on-throw, and the proof that the wrapper forwards to the real scheduler rather than swallowing the frame. The `./test` barrel goes from eight helpers to nine, and its sorted key list is updated to match. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
Nothing is left in it. `getInstance()` is core's, `countRequestedFrames()` is on `./test`, the waits went to `src/test/index.ts` and the todo tree went to `src/todo.fixtures.ts`. A file that exists only to be excluded from the build is one more thing two scripts have to keep true, so both exclusions go with it: - `scripts/build.js` drops the `'!test-utils.ts'` glob. The remaining suffix globs — `.spec`, `.bench`, `.fixtures` — are patterns rather than filenames, so nothing has to be added to them when a helper module appears. - `scripts/check-package.js` drops the `dist/test-utils.` prefix guard and keeps the suffix test that covers the rest. The comment on `todo.fixtures.ts` pointed at `test-utils.ts` for the contrast it drew; it now points at `src/test/index.ts`, and says which part of its own name keeps it out of the build. Verified with the symlink `check:package` needs in a worktree: build emits no `dist/test-utils.*`, the packed tarball is 594 files, and the node, TypeScript and browser packed consumers all pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
DESIGN.md §"The page-wide lookup" described one function with an `$isMounted` filter. It now describes four, says which population each answers, and states the two facts a reader would otherwise have to find by experiment: the map read — not a mount check — is what keeps an inactive declaration out of a result, and the element form is the only one that reaches a detached element. §13 goes from eight helpers to nine and states the global patch `countRequestedFrames()` performs. RATIONALE.md gets a new section under §5, "Why one lookup became four", with the three-step narrowing written out — the argument that dropping the filter resurrects nothing — and the reason `getMountedInstance()` does not exist. §13's "What was refused" list is corrected rather than rewritten, following the precedent `recordEvents()` set there. Two of its four refusals reversed. `getInstance(el, name)` was refused on the ground that `getInstances()` already answered it; it answered a filtered plural version, and 52 spec files importing a private copy are the measurement. The frame counter was refused for "one caller"; it had four by the time the module shipped, and the global patch it was refused for is precisely why a consumer cannot write it. The refusal of a `test-utils.ts` re-export shim is the one that held — right through the file's deletion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #865 +/- ##
=======================================
Coverage 97.06% 97.06%
=======================================
Files 176 176
Lines 4561 4561
Branches 1331 1330 -1
=======================================
Hits 4427 4427
Misses 122 122
Partials 12 12
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:
|
Export sizeBundled per export with peer dependencies left external, dynamic imports excluded and the output minified; sizes are gzipped. @studiometa/js-toolkit-v4
Unchanged (375)@studiometa/js-toolkit
@studiometa/js-toolkit-v4
|
Code ReviewRisk: Low — No concrete defects were found in the reviewed source and packaging changes; the MR is safe to merge aside from the unreviewed files listed below. The MR splits instance lookup into all, mounted, unmounted, and singular APIs, moves Files reviewed: Notes:
Review usage: 107,170 in (74,466 cached) / 1,612 out tokens — $0.0290 (openrouter/openai/gpt-5.6-luna, thinking: low) Reviewed by @weareikko/code-review v0.9.5 for commit 5fdeb90. |
v4 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.
Within noise (17)
|
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.
The problem
getInstances()filtered on$isMountedand said nothing about it at the call site. Two consequences:getInstances('Foo')returning 2 where three elements declareFoo— one below itsmedia:breakpoint — was not debuggable from the name.el[INSTANCES]is marked "Not public API", yet 52 spec files reached around the public surface for a privategetInstance()insrc/test-utils.ts, because the public API could not express its own primitive.migration/REPORT.md:843records the same pattern one level up:Actionhad to "reach past the public surface to write a lookup core could write in ten lines".v4 is unreleased, so the semantics change is free now and never again.
The API
Four exports in
src/instances.ts, over one internalcollect()plus a predicate — the traversal is written once.No
getMountedInstance: the singular returns one object, so a caller reads.$isMountedon it rather than trusting anundefinedthat would mean two things.Dropping the filter resurrects nothing. The string form narrows three times, and the middle step already does the work the last one is credited with:
querySelectorAll(selectorFor(name))— over-matches; includes the responsive attribute spellings.el[INSTANCES]?.get(name)— narrows to constructed. An inactive declaration has no instance, and a breakpoint-withdrawn one is destroyed and deleted from the map atregistry.ts:622.$isMounted— narrowed to live.What step 3 did hide is real and small: the instances a reversible
in-viewormedia:strategy stands down and keeps for the crossing back. That population now has a name.src/utils/selectors.tscredited the narrowing to step 3, so its doc comment is rewritten — a comment that misattributes an invariant is how the filter comes back.Two asymmetries are documented rather than discovered: the element form works on a detached element where the string form cannot reach one from
document, and the string form is DOM order where the element form is mount order.Also here
countRequestedFrames()moves onto./test(eight helpers to nine). Its doc comment states plainly that it patchesglobalThis.requestAnimationFrameand restores it in afinally, and it ships with three tests: the count, the restore-on-throw, and the proof that the wrapper forwards to the real scheduler.src/test-utils.tsis deleted, along with the now-dead'!test-utils.ts'glob inscripts/build.jsand thedist/test-utils.guard inscripts/check-package.js.recordEvents()set there: two of its four refusals —getInstance(el, name)and the frame counter — reversed, and the reasons they were wrong are recorded next to them.Commits
feat(v4)refactor(v4)test(v4)feat(v4)refactor(v4)docs(v4)Every commit was verified green before the next one landed.
Call sites: which one, and why
getMountedInstances(), because the result is used rather than counted:ActionEvent.instances(line 66) — builds the name→instance map the effect calls methods on.ActionEvent.targets(line 102) — a stood-downin-viewtarget must not be run against.Sticky.instances(line 70) — not in the brief, found by grep. It stacks siblings by index and sums their heights; an unmountedStickyhas no position to contribute.mount(),resetDom(),recordEvents()andresetRegistry()specs onsrc/test/index.spec.ts, andresponsive-components.spec.ts:112— all assert "this component is live", whichgetInstances()no longer says.getInstances(), kept, because the claim is "nothing was ever built" and it is now the stronger assertion — no filter can be hiding an instance:autoload.spec.ts:155on a lazy declaration before its class arrives.resetRegistry()assertions onsrc/test/index.spec.ts.Verification
Run in
packages/v4unless noted.check:packagehardcodes../../node_modules/typescript/bin/tsc, so the main checkout'snode_moduleswas symlinked into the worktree root for that run and removed after; the tree is clean.Tests 1558 → 1573; nothing was deleted. Root barrel exports 86 → 89, updated in
src/exports.spec.ts:186andtest/package-node-consumer.js:62. Subpath stubs and theexportsmap were regenerated withnpm run subpaths, never hand-written.Notes for the reviewer
T | undefinedreturn is where the churn is. The private helper was(el: Element | null, name: string): T, which lied twice — it took aquerySelector()result and claimed the instance was always there. The public one is honest, so 202 call results and 58querySelector()arguments now say!where the spec knows better. Where the spec was already handling the absence —toBeUndefined(),toBeTruthy(),Boolean(...), awaitFor()predicate polling for the instance — no assertion was added.Carousel.spec.ts > re-normalises the index when the slide it points at is removedfailed once mid-run and passed on the file alone and on three later full-suite runs. It is scroll-observer timing under load and touches no code in this PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM