Repository navigation
fix(signals): a failed derived-store refetch reaches Errored (#3887) - #3917
Merged
Merged
Conversation
pullFamily let a held render effect outside the flight's flush keep its frame when the derive had errored too; a rejection has no landing to learn of, so the error never reached the boundary. Co-authored-by: Claude via Cursor <noreply@cursor.com> Co-authored-by: Cursor <cursoragent@cursor.com>
🦋 Changeset detectedLatest commit: 2f412ef The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size (brotli, eager entry chunk)
|
Coverage Report for CI Build 37798443014Coverage remained the same at 76.43%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
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
2 participants
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.
Fixes #3887
Thanks to @ethan-huo for the report.
Root cause
Before a derived store serves a value,
pullFamily(packages/signals/src/store/store.ts) brings its derive, the family's firewall node, up to date. When the derive is pending or errored, the reader links to it and observes it like a memo reader would. The exception is a render effect outside the flight's own flush: it "keeps what it shows" and returns early, learning of the landing from the leaves the landing changes. That's right for a flight, which lands and changes leaves. But the same early return also fired when the derive had errored. A rejection has no landing and changes no leaves, so the render effect never read the error, andErrorednever saw it. The UI kept showing the last value.This hits any
createStore(fn)whose re-fetch rejects: sync derive to rejecting async, and async to rejecting async. A plain memo reader has no such shortcut, so memos are fine.Fix
Add
!(fw._statusFlags & STATUS_ERROR)to the keep-the-frame condition, so an errored derive is read (readNode(fw)throws its error) and the error reaches the boundary. Pending derives behave exactly as before.Test
packages/signals/tests/store/derived-refetch-error-3887.test.tsmounts the JSX shape at the signals level: a render effect reading the store under a loading boundary under an error boundary, with an outer render effect recording the boundary's output."error". Fails onnext(stays"content")."error". Fails onnext.I also ran the triage's JSX repros (
<Errored><Loading><span>{store.value}</span>) against the fix in@solidjs/web, and all three pass. The full signals suite passes.Related: draft #3886
@GabbeV's draft #3886 ("publish value and error outcomes consistently") also fixes this, as part of a much larger rework of how value and error outcomes publish. This PR is the minimal hotfix for rc.15 and doesn't touch #3886. If #3886 lands, its rework may subsume this condition. The test here is written against observable behaviour, so it should carry over unchanged to reconcile the two.
Size
From CI's Size job (Rolldown, brotli, eager entry chunk, vs base
next@ 8d23a5a):signals: + createStoreapp: hydrating + every store primitive familyapp: compiled CSR(store)app: compiled hydrating(store)app: render + one signal(hello world) andsignals: core floorThe gate passes. The other⚠️ page warnings are identical on
next.Public API changes
None.