Repository navigation
Conversation
…st children change React reuses an <Entity> or <EntityList> across conditional branches that differ only in their children. The collection was memoized on entity type and by/options alone, so the new children rendered against the previous selection and threw UnfetchedFieldError. The children function is now part of the memo key; a store-driven re-render keeps the same children and does not collect again, and an unchanged query keeps the same result object. Fixes #154
Re-collecting on every new children function made a field toggled by parent state refetch on every toggle, and hiding a field shrank the query. The selection now only grows: children covered by the cached selection return it unchanged, and new fields are merged into a deep copy so neither the cached selection nor component selections are mutated.
…d registry Neither Entity's by key nor EntityList's options key names the entity, so an EntityList reused for another entity with the same options merged the previous entity's fields into its query. The cache now records entity type, depsKey and schema registry, and grows only when all match. With a compiled selection the runtime walk no longer grows, so validate mode diffs against what the current children read.
…der new props An equal query under another entity, depsKey or registry now returns a new cache object instead of rewriting the shared one. A skipped compiled walk passes no source, so a new children identity does not re-run it. Covers a by change with a test that the selection starts afresh.
matej21
left a comment
matej21
left a comment
There was a problem hiding this comment.
Review round 1: review agent (Claude Opus 5.5), code reading only (tests not run), commit 63ee3b03. Verdict: changes.
The #154 fix is correct and the branch-switch tests cover it directly. One issue with the "selection only grows" rule needs a bound before merge.
Major: the grow rule never stops for has-many params. useSelectionCollection.ts:145-162 together with HasMany.tsx:95-103.
<HasMany>gives every distinctfilter/orderBy/limit/offsetits own alias (generateHasManyAlias). A new param set is therefore never "covered": it is always added, and old aliases are never dropped.- Failure scenario:
<Entity by=…>{a => <HasMany field={a.comments} filter={{ text: { containsCI: search } }} limit={10} offset={page * 10}>…}</Entity>, withsearchorpageheld in parent state. Every keystroke or page change adds a new alias, and the refetch carries every earlier subquery as well (comments_x1,comments_x2, …). The query and the cached selection grow for as long as the component is mounted. depsKeycovers onlybyand the list options, never nested params.- Suggested fix: shrink when a has-many alias's params are replaced, or grow by field name and drop superseded aliases. Add a test.
Minor: "store-driven re-renders keep the same children" holds only for the entity's own subscription. useRootSelection.ts:83, EntityList.tsx:83.
- A nested
<Entity>/<EntityList>inside another entity's children gets a new inlinechildrenon every outer re-render, and the outer entity re-renders on every store change, including keystrokes. Each render then runschildren(collector)andcollectSelectionagain. - There is no refetch, because the covered check short-circuits. The cost is CPU per keystroke on large nested forms.
- Worth a changelog note, or a cheaper check before the walk.
Minor: switching branches now fails silently instead of throwing (#155). Newly needed fields read as empty until the refetch lands, so in an edit form a user can type into an input that looks empty while the real value is still loading. Worth scheduling #155 together with this PR.
Nit: isSelectionCovered ignores the isRelation / isArray flags (useSelectionCollection.ts:75), while mergeSelections uses them. It is harmless today.
Nit: in validate mode with grow: false and an inline source, runtimeResult changes whenever a toggled field changes the query, so the diff effect can repeat the same warnings.
Compiler path: no conflict.
- It passes
source: undefinedand the::compiledkey, so the walk never re-runs. - The babel plugin hoists the compiled selection per JSX site, so a branch switch gets a new
compiledSelectionand its memo recomputes. - The semantics differ by path: the compiled selection is replaced on a switch and can shrink, while the runtime selection only grows. Worth documenting.
Missing tests:
- has-many params changing (the major above);
- a nested
<Entity>under an outer entity that re-renders from store changes; - validate mode across a branch switch (already noted in the PR).
… instead of growing <HasMany> gives each filter/orderBy/limit/offset its own alias, so under the grow-only rule a search or page held in parent state added a subquery on every change. A cached has-many alias of a field the collected children read under other parameters is now dropped before the merge. The covered check also compares the relation flags, and the docs spell out the nested-Entity collection cost and that a compiled selection does not grow.
… bring a new one for its field Any read of the same field, including the plain one the collector records, counted as a replacement, so unrelated growth dropped a filtered list that was only hidden. Aliases are now dropped only when the collected children read the field under parameters the cache has not seen, and not at all when the JSX analysis failed. The filter test also checks that the newest alias is the one fetched.
|
Howdy, partner 🤠 Thanks for ridin' through this one. Here's how the herd got sorted. Major: has-many params stampede. You're dead right, that cattle would've grazed till the end of time. Fixed in 65f443a (dcab456 had the first, too-greedy pass):
Minor: nested Minor: silent empty instead of a throw (#155). Agreed, that's the next trail I'm takin'. I kept it out of this PR on purpose: loading-until-covered versus keep-the-old-children is a design call, and I'd like your word on it in #155 before I build it. Nits:
One more thing I spotted out on the range, outside this PR: Still to rope: no validate-mode test across a branch switch yet (noted in the PR body). Happy trails 🐎 |
|
Opened that one as #158, with a failing repro on |
Fixes #154.
Problem
React reuses an
<Entity>/<EntityList>across conditional branches that differ only in theirchildren(same entity, sameby/ list options).useSelectionCollectionmemoized the collection on[entityType, depsKey, schemaRegistry]and readcollectthrough a ref, so the new children rendered against the previous selection and threwUnfetchedFieldError. Seen in production on a tabbed detail page.Change
useSelectionCollectiontakes asource(the userchildrenfunction), passed byuseRootSelectionandEntityList; a new identity runs the collection again. Store-driven re-renders keep the samechildren, so they do not.depsKeyand schema registry the selection only grows (anEntityListreused for another entity with the same options starts afresh).<HasMany>aliases every filter / orderBy / limit / offset) replaces the cached aliases of that field the children no longer read, so a search or page held in parent state swaps its subquery instead of piling up. Without such a new alias nothing is dropped, so a hidden filtered list survives growth elsewhere; after a failed JSX analysis nothing is dropped either. Children covered by the cached selection get the same result object back (no refetch, no new handle), so a field toggled by parent state or a branch switched back and forth fetches each field once, and hiding a field never shrinks the query. New fields are merged into a deep copy, so neither the cached selection held byuseEntitynor component selections are mutated.Tests
tests/react/jsx/entityBranchSwitchSelection.test.tsx:<Entity>and<EntityList>branch switch with the sameby/ options (both fail onmainwith the reported error);<EntityList>reused for another entity does not carry the previous entity's fields into its query;<Entity>switched to another record starts its selection afresh;<Entity>is not refetched when the outer entity re-renders on a store change;With a compiled selection, the runtime walk (validate mode only) does not grow, so the under-fetch diff compares against what the current children read.
bun run test: 2532 pass.bun run typecheckclean.Known gaps
No test yet for validate mode across a branch switch (the compiled path has its own tests; the runtime walk there no longer grows).
Until the refetch for newly needed fields lands, they read as empty instead of loading: Entity renders never-fetched fields as empty data while its selection grows #155. This already happened on the compiled path and on the first mount of a nested
<Entity>; this PR makes it reachable from a runtime branch switch, where it used to throw.Every parent re-render with an inline
childrennow runs the collection (the result stays cached). Thesourcedoc recommends a stablechildrenunder parents that re-render often.EntityCreateModeInnerstill runsuseRootSelectionand ignores the result; it now does so on every parent render. Left as is.