Add retry and getUnifiedState to ReactiveStore#1552
Merged
Conversation
🦋 Changeset detectedLatest commit: cb4c655 The changes in this PR will be included in the next version bump. This PR includes changesets to release 47 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 |
BundleMonFiles updated (13)
Unchanged files (134)
Total files change +2.5KB +0.48% Final result: ✅ View report in BundleMon website ➡️ |
Member
Author
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Nice, focused evolution of ReactiveStore toward the ActionStore shape. The unified { data, error, status } snapshot with stable identity is exactly what useSyncExternalStore needs, and preserving the existing getters (deprecated) keeps this non-breaking. Factoring the factory variant out as a separate function rather than overloading the existing one is the right call — the semantics of retry() differ enough to justify the split.
One correctness bug worth fixing before merge, plus a few smaller notes below.
Things to watch out for
retry()after the outer abort leaves the store stuck inretrying(see inline). The guard is inconnect()butretry()has already transitioned state by the time it's hit. Both the factory variant and the slot-tracking variant have this.- Asymmetric
retry()behaviour across factories. The deprecatedcreateReactiveStoreFromDataPublisherthrows fromretry(); the factory variant is a silent no-op when not inerror. Defensible (the throw surfaces the deprecation loudly), but it means a React component that wiresretryto a button can crash if it happens to be rendered against a non-retryable store. Worth flagging more prominently in the README snippet for the deprecated function. lastUpdateSlotis preserved across retry in the slot-tracking store (intentional and documented). Be aware this means a retry whose RPC + subscription both initially return slots< lastUpdateSlotleaves the store stuck inretryinguntil the subscription catches up. In practice that window is tiny, but it's worth a brief comment at the site of the slot check.
Notes for subsequent reviewers
- The inner/outer abort-controller pattern in the factory variant is the trickiest part to eyeball. Specifically worth re-checking: (a) stale listeners from a prior connection are aborted before
retry()wires up the new one, and (b) theouterController.signal.addEventListener('abort', forwardAbort, { signal: innerController.signal })self-unregistration pattern (so we don't leak one listener per retry onto the outer signal). Both looked correct to me, but they're load-bearing. - Test coverage for the new API is thorough across all three sites. The one gap I'd suggest is that the "retry after abort is a no-op" tests only assert call counts, not the resulting
status— which is why they pass despite the state-machine bug flagged below. - Changeset bump is
minoracross@solana/errors,@solana/subscribable,@solana/kit— matches the CLAUDE.md convention for new APIs. The existing getters are only@deprecated(not removed), so this is additive. - Error code
SOLANA_ERROR__SUBSCRIBABLE__RETRY_NOT_SUPPORTED = 8195000opens a new reserved range[8195000-8195999]for theSUBSCRIBABLEdomain. Looks consistent with the existing registry layout.
| @@ -105,7 +117,9 @@ Things to note: | |||
|
|
|||
| ### `createReactiveStoreFromDataPublisher({ abortSignal, dataChannelName, dataPublisher, errorChannelName })` | |||
|
|
|||
There was a problem hiding this comment.
Minor, but worth making the crash behaviour more prominent here: readers might skim the note and assume "deprecated but still works everywhere", then wire onRetry={store.retry} to a button (matching the React example in the ReactiveStore<T> section above) and get an uncaught SolanaError at click-time.
Consider hoisting the "calling retry() throws" clause to its own sentence (or bold fragment) so it reads more like a warning than a footnote, or mention it in the React example for the type itself. Not a blocker.
Member
Author
There was a problem hiding this comment.
I think this is clear enough in practice
Contributor
|
Documentation Preview: https://kit-docs-qxqgoeesb-anza-tech.vercel.app |
mcintyre94
force-pushed
the
reactive-store-updates
branch
2 times, most recently
from
13959e2 to
4baf015
Compare
Member
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Apr 22, 2026
lorisleiva
approved these changes
Apr 22, 2026
Member
lorisleiva
left a comment
lorisleiva
left a comment
There was a problem hiding this comment.
Reviewed this the wrong way around but I get what you're doing haha. Nice changes!
mcintyre94
force-pushed
the
reactive-store-updates
branch
from
4baf015 to
1d06dcc
Compare
`ReactiveStore<T>` now exposes a `getUnifiedState()` method returning a discriminated `{ data, error, status }` snapshot over `loading | loaded | error | retrying`. The snapshot has stable identity across reads, so stores can be passed directly to `useSyncExternalStore` without an intermediate wrapper. `getState()` and `getError()` remain on the type but are deprecated in favour of the unified snapshot.
A new `createReactiveStoreFromDataPublisherFactory` accepts a `() => Promise<DataPublisher>` factory and supports real `retry()` by wiring a fresh inner abort signal for each connection attempt. The existing `createReactiveStoreFromDataPublisher` is deprecated; its `retry()` throws a new `SolanaError` with code `SOLANA_ERROR__SUBSCRIBABLE__RETRY_NOT_SUPPORTED` because a ready-made `DataPublisher` can't be restarted.
In `@solana/kit`, `createReactiveStoreWithInitialValueAndSlotTracking` now implements `retry()` by re-invoking the pending RPC request and subscription on a new inner abort signal, preserving `lastUpdateSlot` and the last known value.
mcintyre94
force-pushed
the
reactive-store-updates
branch
from
1d06dcc to
cb4c655
Compare
Member
Author
Merge activity
|
Contributor
|
🔎💬 Inkeep AI search and chat service is syncing content for source 'Solana Kit Docs' |
This was referenced May 7, 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
3 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.
Problem
ReactiveStoreexposedgetState()andgetError()as separate getters with no unified snapshot, which madeuseSyncExternalStoreintegration awkward. Errors were also terminal — once a stream failed, the only recovery was to rebuild the whole store, losing subscribers and last-known state in the process.Summary of Changes
This PR brings
ReactiveStorecloser to the proposedActionStore: #1550. See discussion: #1550 (review)ReactiveStore<T>gainsretry(): voidandgetUnifiedState(): ReactiveState<T>.ReactiveState<T>is a discriminated union overloading | loaded | error | retryingwith stable snapshot identity across reads. The existinggetState()andgetError()getters are preserved but marked@deprecated. A future breaking change can makegetState()the newgetUnifiedStatecreateReactiveStoreFromDataPublisherFactoryaccepts a() => Promise<DataPublisher>factory and supports real retry, wiring a fresh inner abort signal for each connection attempt.createReactiveStoreFromDataPublisheris now@deprecated. Itsretry()throws a newSolanaErrorwith codeSOLANA_ERROR__SUBSCRIBABLE__RETRY_NOT_SUPPORTEDsince a ready-madeDataPublishercan't be restarted.createReactiveStoreWithInitialValueAndSlotTracking(in@solana/kit) now implementsretry()by re-invoking the pending RPC request and subscription on a new inner abort signal, preservinglastUpdateSlotand the last known value.