test(solid-query): pin the stale read through a memo over query.data (#11351) 🤖🤖🤖 - #11357
Open
mgarcialeniolabs wants to merge 1 commit into
Open
test(solid-query): pin the stale read through a memo over query.data (#11351) 🤖🤖🤖#11357mgarcialeniolabs wants to merge 1 commit into
query.data (#11351) 🤖🤖🤖#11357mgarcialeniolabs wants to merge 1 commit into
Conversation
A tracked computation that reaches a leaf THROUGH a `createMemo(() => query.data)` indirection is not re-notified after a refetch, so its last observation stays the superseded value permanently. A wrapper hook that gates or narrows `query.data` before handing it to consumers is exactly this shape, so it reaches app code that never writes the memo explicitly. The data node is not at fault: the derive runs the expected number of times, the commit lands, the DOM swaps, and an untracked read through the very same memo returns the new value. Every other reader shape is notified correctly, and the behaviour reproduces with no TanStack code involved, so the test is skipped pending the upstream fix in solidjs/solid#3181. Refs TanStack#11351 Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
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.
🎯 Changes
Adds a reproduction for #11351 to
useQuery-semantics.test.tsx, in thedata store faceblock.A tracked computation that reaches a leaf through a
createMemo(() => query.data)indirectionis not re-notified after a refetch, so its last observation stays the superseded value permanently.
A wrapper hook that gates or narrows
query.databefore handing it to consumers is exactly thisshape, so it reaches app code that never writes the memo explicitly.
The test is added as
it.skip, following the existing convention inuseQuery.test.tsxfor adocumented divergence, because the fix is not in this repo — see below. Un-skipped it fails on
the last assertion only, after
expect(rendered.getByText('flag: true'))has already passed: thecommit lands and the DOM swaps, and only the reader behind the memo is starved.
Why no fix here
The data node is not at fault.
computeDataruns exactly the expected four times across mount andrefetch,
chainOnceproduces one chained promise per fetch with no identity churn, the commitlands, an untracked read through the very same memo returns the new value, and neither the memo nor
the data node is left
isPending. Only the notification to one reader shape is lost.Narrowing across reader shapes (all against the real hook):
query.data.flagquery.data.flag, effect reads the memoquery.dataviauntrack, then reads.flagoff itquery.data.flagquery.data, effect readsmemo().flagquery.data, effect reads bothquery.dataandmemo().flagRow 6 is the one that rules out a missing subscription: the effect reads the projection directly
and depends on the memo, and is still not notified. Row 4 rules out "any suspended memo
dependency" — the memo has to be a consumer of this projection. A memo created after the first
settle is fine; only one that suspended on its first run is affected.
The behaviour reproduces with no TanStack code involved, so I filed it upstream:
solidjs/solid#3181.
2.0.0-rc.4is the newest publishedsolid-js/@solidjs/signals, sothere is no version bump that fixes it.
The workarounds I tried and rejected, in case they come up in review:
reconcile-off, and discards the fine-grained leaf tracking pinned by
'tracks deep reads at the leaf — unrelated changes do not re-run them'.get data()— works only if the getter drops itssuspending read, which would stop
query.datasuspending into<Loading>and break primitiveand array data.
by
transition.test.tsx.landedflag onchainOnce, returning the unlanded chained promise instead ofwrap(state.data)) — tried andreverted: it does not fix the reproduction and it breaks
'keeps stale data and exposes error state when a refetch fails'.Happy to drop the skipped test and just leave the narrowing on the issue instead, if you'd rather
not carry it.
✅ Checklist
pnpm run test:pr, or these tests do not apply to this pull request.Verified locally from the repo root:
Baseline on
solid-query-v6-prebefore this change is 346 passed / 1 skipped, so the only delta isthe added skipped test.
🚀 Release Impact
Made with Cursor