refactor(v4): finish moving the specs onto the shipped test helpers - #866
Merged
Conversation
`getInstance()` read `el[INSTANCES]?.get(name)`, a body that already tolerated a missing element, behind an `el: Element` parameter that did not. Every caller holding a `querySelector()` result had to assert the element away with a `!` — 58 of them across the specs — to state something the function never needed. The parameter is now `Element | null`. "No element" and "no instance" are the same answer to the caller, and `undefined` says it for both. The return stays `T | undefined`: an absent element is a fact the caller may reasonably not know, an absent instance is one it must handle. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
`INSTANCES` is documented as not public API, so a spec spelling `el[INSTANCES]?.get(name)` was reaching past the lookup written for it. Every such read across ten spec files now calls `getInstance()`, which the widened parameter makes ergonomic on a `querySelector()` result. Three local `instanceOf` / `instance` wrappers went with them, and `.has()` reads became `toBeUndefined()` on the lookup. Three uses stay and now say why: - `Base.spec.ts` writes with `.set()` to file one instance under a second name; `getInstance()` only reads. - `instances.spec.ts` reads the map to state that none exists, which is the premise its assertions rest on — and testing these exports with themselves would prove nothing. - `coexistence.spec.ts` has the symbol key itself under test, against v3's `__base__`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
Fourteen spec files attached their own listener to the diagnostic
channel, each re-deriving the same three lines — push the detail, cancel
the event, remember to detach. `captureDiagnostics()` from `./test` is
that, written once, so they now call it.
Three real defects went with the duplication:
- `attribute-namespaces.spec.ts` never removed its listener, so it
leaked into every later test in the file.
- `swap.spec.ts` had a local helper named `captureDiagnostics` which did
*not* cancel, so its warnings reached the console during the run.
- `FigureVideoTwicpics.spec.ts` waited six settle rounds for a
diagnostic that `waitFor` now polls for.
`scheduler.spec.ts` also drops three `{ once: true }` listeners, which
turns "the first diagnostic was this one" into "exactly this one
arrived".
Left hand-rolled, and now saying so in the file:
- `diagnostics.spec.ts` owns the channel. Six of its seven listeners
assert cancellation itself, the order against the console sink, the
target a dispatch started on, or detail mutation — all of which
`captureDiagnostics()` either performs or hides. The seventh only
counts, and converted.
- `mount-strategies.spec.ts` asserts `{ target: el, defaultPrevented }`
on the event object, which the helper does not hand back.
- `autoload.spec.ts` asserts the same diagnostic reaching an element,
`document` and `window`, which needs three listeners.
- `Base.spec.ts` asserts which element a diagnostic started on.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
Nine more spec files kept a raw listener even though they already imported `captureDiagnostics()` for their other tests. Each converted site only collected details, so the helper is a straight replacement. Two of them shadowed the shipped helper with a local function of the same name — `storage/createStorage.spec.ts` and `storage/providers.spec.ts` — which is the one thing a reader cannot resolve at a glance. Both are now `catchDiagnostics()`, built on the shipped capture and named for what they add: a bounded run. `negotiated-events.spec.ts`, `services/service.spec.ts` and `services/mutation.spec.ts` gained the same shape. Every listener left raw asserts something the helper does not hand back — the event's `target`, its `defaultPrevented`, or three targets at once for a propagation test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
Three specs re-implemented the poll: `Toaster.spec.ts` and `Toast.spec.ts` each carried a `waitFor` with the same signature as the shipped one, and `viewTransition.spec.ts` spun 50 microtasks in `waitForBatch`. All three now call `waitFor()`, which advances the framework's own scheduler between attempts instead of spinning. `services/pointer.spec.ts` also had a local `frames()` shadowing the shipped one, differing only in its default count. The one call site relying on that default now names the count it needs. `FetchShopifyPartial.spec.ts` defined a local `async mount(html)` shadowing the shipped `mount`, so a reader could not tell which one a call resolved to. It is now `mountWithInstance()`, built on `mount()` and named for what it adds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
`TimerProgress.spec.ts` was the one remaining hand-rolled event recorder: a listener pushing `detail.ratio` into an array. It now projects `recordEvents()`, the way `Timer.spec.ts` already did. The other four recorders in the audit stay, and each has a reason: - `attribute-namespaces.spec.ts` records binder callbacks, not events. - `services/mutation.spec.ts` records service props off `subscribe()`, also not events. - `Base.spec.ts` records the element each diagnostic *started on*, which `recordEvents()` and `captureDiagnostics()` both drop. - `Timer.spec.ts` already wraps `recordEvents()`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
`getInstance` takes `Element | null` now, so narrowing a `querySelector()` result before passing it asserts something the signature already allows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
Export sizeBundled per export with peer dependencies left external, dynamic imports excluded and the output minified; sizes are gzipped. @studiometa/js-toolkit-v4
Unchanged (397)@studiometa/js-toolkit
@studiometa/js-toolkit-v4
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #866 +/- ##
=======================================
Coverage 97.06% 97.06%
=======================================
Files 176 176
Lines 4561 4561
Branches 1330 1331 +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:
|
Code ReviewRisk: Low — The refactor is safe to merge; no concrete defects were found in the reviewed changes. The MR updates the v4 specs to use the shipped Reviewed all 47 staged diff files listed in the request, including Review usage: 89,439 in (16,150 cached) / 2,144 out tokens — $0.0530 (openrouter/openai/gpt-5.6-luna, thinking: low) Reviewed by @weareikko/code-review v0.9.5 for commit e4e453f. |
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. No benchmark moved beyond the noise floor. Within noise (18)
|
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.
Finishes the move onto the shipped
./testhelpers, so that every remaining hand-rolled instance is a decision rather than an oversight.getInstancetakes a nullable elementgetInstance<T>(el: Element | null, name: string): T | undefined. The body was alwaysel[INSTANCES]?.get(name), so it already tolerated null — the narrow parameter only forced callers to assert. "No element" and "no instance" both honestly answerundefined.That removed 29 dead non-null assertions on
querySelector()arguments. The ~202!on results stay:T | undefinedis the correct return type, and stripping it at a call site that just mounted the component is a real claim about a real thing.Converted
js-toolkit:diagnosticlisteners in 12 spec filescaptureDiagnostics()INSTANCESsymbol reads inswap.spec.ts(6) andBase.spec.ts(1)getInstance()Toaster.spec.ts,Toast.spec.tswaitFor()Timer.spec.ts,TimerProgress.spec.tsrecordEvents()waitFor()mount(html)inFetchShopifyPartial.spec.tsshadowing the shippedmountDeliberately kept, with reasons
These are the exemptions, not the leftovers.
Raw
INSTANCESaccess — 3 files.Base.spec.ts:1332—[INSTANCES]?.set(...). A write. No accessor expresses it.instances.spec.ts— assertsel[INSTANCES]isundefined, i.e. the map was never created.getInstance()returningundefinedis the weaker claim that the map exists with no entry, so the raw read is the right tool in the module's own spec.coexistence.spec.ts— exists to test v3 and v4 sharing a document throughSymbol.for, and reads[...keys()]. The symbol is the subject.consolespies — 5 sites.Action.spec.ts:61,74— the action's expression is literallyconsole.log(ctx). The spy is the assertion target.DataDerived.spec.ts:97,124andDataBind.spec.ts:298— assert aconsole.errorreached the sink.diagnostics.spec.ts,negotiated-events.spec.ts,Base.spec.ts($warn/$error) — assert about the sink: does cancelling suppress it, does the shorthand reach it.captureDiagnostics()cancels every event, which would make each of these vacuous.Base.spec.ts's own diagnostic recorder — capturestargets: EventTarget[], i.e. which element the diagnostic started on.captureDiagnostics()takes a single target and does not expose that.events.spec.ts— asserts the channel's event name, not what travels on it.The 9 remaining settle-loops, renamed to
quiet(). These back negative assertions — "nothing loaded", "nothing mounted yet". They cannot becomewaitFor, because polling for an absence passes before anything has happened. This is the asymmetry recorded inREPORT.md:900and inwaitFor's own doc comment. The rename replaces four names (observed,settled,included,ready) with one that says what the helper is for, and each carries a comment saying why it is not a poll.The ~47 local
render()helpers — they build component-specific markup, which no shared helper can do. Only the identical DOM-plus-settle copies were ever duplication, and those are gone.mutation.spec.ts'srecordFor()andattribute-namespaces.spec.ts'srecorder()— neither records DOM events. One builds aMutationRecordfixture; the other collects qualifier callbacks. Name collision only.Verification
packages/v4npm run testpackages/v4npm run lint:typespackages/v4npm run check:diagnosticspackages/v4npm run buildpackages/v4npm run check:packagenpm run subpaths:checknpm run lint:fmtTest count unchanged at 1573 — this PR moves specs onto shipped helpers, it does not add or remove coverage.
🤖 Generated with Claude Code
https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM