feat(v4): add createGroup, the missing half of v3's withGroup - #826
Merged
Conversation
Code ReviewRisk: Medium — issues that should be addressed before merge. Adds and exports 1 issue found:
Review usage: 29,309 in (3,056 cached) / 667 out tokens — $0.0186 (openrouter/openai/gpt-5.6-luna, thinking: low) Reviewed by @weareikko/code-review v0.9.5 for commit 842427d. Previous review runsPrevious run archived 2026-08-16T11:48:58ZCode ReviewRisk: Low — The change adds the Review usage: 15,918 in (3,056 cached) / 484 out tokens — $0.0096 (openrouter/openai/gpt-5.6-luna, thinking: low) Reviewed by @weareikko/code-review v0.9.5 for commit 22edb6d. |
Export sizeBundled per export with peer dependencies left external, dynamic imports excluded and the output minified; sizes are gzipped. @studiometa/js-toolkit-v4
Unchanged (383)@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 #826 +/- ##
=======================================
Coverage 97.16% 97.16%
=======================================
Files 170 170
Lines 4133 4133
Branches 1152 1152
=======================================
Hits 4016 4016
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:
|
Merging this PR will degrade performance by 21.7%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | translate3d only (x, y, z) |
294.4 µs | 376 µs | -21.7% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing feature/v4-grouping (842427d) with main (ae3c716)
Footnotes
-
141 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
v3's `withGroup` gave each instance a `$group: Set<Base>` keyed by a group name on a page-global registry. It is not ported: the set had no value cell, so a coordinator could read its peers but never learn that one arrived — and v4 mounts on DOM insertion with no ordering guarantee, which makes "the set of my peers" a moving target. `createGroup()` publishes the roster as one `Signal`, in document order. `join()` returns its leave function, which is already the shape of a `subscribeContext()` answer and teardown, so a member joins the nearest group whenever it appears and leaves it before a nearer one takes over. Scoping comes from nearest-provider-wins, so a nested group takes its own members — the case `$watchChildren` cannot express. The spec builds the Disclosure pattern end to end from this helper plus `$provide` and `subscribeContext` alone, and asserts both late-peer orders: a disclosure written open that mounts late loses to the one before it in the DOM and wins over the ones after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011nNdFD3aQhzfdm3EsCSbS9
titouanmathis
force-pushed
the
feature/v4-grouping
branch
from
August 16, 2026 11:48
22edb6d to
873207d
Compare
Section 5 already said `withGroup` is not ported but left open where membership itself lives. Answer it: a roster is a value published through the context system, not a second registry — and note the line the two v3 consumers divide on, since `Data*` groups by name over a keyed store and keeps its own records while `Disclosure` groups by ancestry and takes the helper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011nNdFD3aQhzfdm3EsCSbS9
titouanmathis
force-pushed
the
feature/v4-grouping
branch
from
August 16, 2026 11:48
873207d to
842427d
Compare
Contributor
Author
|
The Verified on the reviewed commit itself ( The PR's file set is eight files, all under The symlink was mine — I created it so the agent worktree could resolve dependencies, and an amend swept it into the commit. It was removed before this head was pushed. Merging. |
This was referenced Aug 16, 2026
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.
Verdict: one small primitive was missing, and it is not
$groupContexts cover finding the group. They do not cover being the group.
provide/inject+subscribeContext()already solve the hard half of v3'swithGroup: nearest-provider-wins gives a member its closest group, a pending request is replayed when a provider appears later, and the mount announcement re-answers a member when a nearer group shows up. That is the whole of Disclosure'sDISCLOSURE_CONNECTED/DISCLOSURE_GROUP_CONNECTED/useMutation/__connect/__disconnectdance, deleted.What nothing in v4 provides is the roster on the coordinator's side:
$watchChildrenis the closest thing, and it is wrong here: it collects every matching descendant across nested group boundaries, so a nestedDisclosureGroup's items land in the outer group's collection. Disclosure has to filter them back out by asking each item which group it belongs to.Set— what v3 handed out — has no value cell. A coordinator can read its peers but never learn that one arrived. With v4 mounting on DOM insertion and no ordering guarantee, an invariant over the set (one open at a time) has to be re-checked on every membership change, not established once.So:
createGroup()— 90 lines, one new runtime export, built onsignal()and reached as a context value. No parallel registry; the report's argument against one stands and this does not touch it.Two design points worth arguing about:
join()returnsleavebecause that is already the shape ofsubscribeContext()'s answer/teardown contract. A member's whole group wiring is one call, and migration to a nearer group leaves the old one before joining the new one for free.The Disclosure pattern in v4
Whole thing, no decorators, no build step. This is
src/group.spec.tsverbatim.Against v3's
Disclosure.ts+DisclosureGroup.tsthis drops: two document-levelCustomEventchannels and their listeners,useMutation(document, …)with its per-instance key,__connect/__disconnect/register/unregister,__scheduleReconcileand itsnextTickdebounce, and theitemsgetter's own DOM sort andcontainsfilter. What is left is the invariant, which was always the only part that belonged to the group.What
group.spec.tsasserts13 tests in real Chromium. The ones that matter:
data-componentadded after its members mountedsubscribeContext()re-answer and not a destroy/remountRemoving the
members.subscribe()reconcile fails exactly the two late-peer tests and nothing else, so the roster-as-a-value is load-bearing and not decoration.How the two
Data*sites translateThey already are translated, in
packages/v4/migration/Data/— and they do not usecreateGroup(). That is the interesting part, so spelling it out:DataScope(v3:getScopedGroups(this)+ aWeakMapof scope → name →Set) becomes one field initializer:registry = this.$provide(DataRegistryContext, new DataRegistry({ scoped: true, … })). The scope resolution, theWeakMapand the page-global fallback all collapse into nearest-provider-wins plusprovideRootContext().DataBind(v3:withGroup(Base, 'data:', { getScope, getGroup })andthis.$group) becomes onesubscribeContext()inmounted(): the callback joinsregistry.join(group, this), the teardown leaves, andmounted()returns the unsubscribe.getDataScope()'s manual__base__ancestor walk is gone.DataRegistrykeeps its own membershipSetrather than aGroup, and that is the right call: the two v3 consumers ofwithGroupdivide on what "group" means.Disclosuregroups by ancestry — the group is a component in the DOM, and the whole difficulty was the two sides finding each other.Data*groups by name, with the nearestDataScopeonly choosing which partition table to read; membership is one field of a per-name record beside values, sources and hydration state, and nothing ever subscribes to a roster. It has the identicaljoin(group, member) → leaveshape for the identical reason. A group that is a set of peers getscreateGroup(); a group that is a partition of a keyed store keeps the store. DESIGN.md §5 now records that line.What is deliberately not here
$group. Nothing implicit, nothing global, no name option. The name a v3 consumer passed was the partition key of a page-global registry; a group whose membership is a DOM fact needs neither a name nor a scope resolver.Basemethod.createGroup()importssignaland nothing else — no component graph on the./createGroupsubpath — and the member side is asubscribeContext()call that already has a home.$groupbecause membership lived on a registry that outlived the instance. v4 destroys a component when its element leaves the DOM, so the member's own teardown removes it. Asserted.Verification
npm run lint,npm run lint:types,npm run test:v4(988 passed) andnpm run check:packageare green on each commit. Root runtime exports go 79 → 80;packages/v4/src/subpaths/createGroup.tsand the exports map are generated bynpm run subpaths.🤖 Generated with Claude Code
https://claude.ai/code/session_011nNdFD3aQhzfdm3EsCSbS9