Add ReactiveActionStore to @solana/subscribable#1550
Conversation
🦋 Changeset detectedLatest commit: 40afbcb 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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
BundleMonFiles updated (3)
Unchanged files (144)
Total files change +1.06KB +0.2% Final result: ✅ View report in BundleMon website ➡️ |
|
Documentation Preview: https://kit-docs-9izar8b4r-anza-tech.vercel.app |
mcintyre94
force-pushed
the
graphite-base/1550
branch
from
f5f38cb to
f370c6c
Compare
mcintyre94
force-pushed
the
action-store
branch
from
b1a0140 to
0cbddad
Compare
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Adds createActionStore to @solana/subscribable: a framework-agnostic state machine (idle / running / success / error) that wraps an async function and exposes { dispatch, getState, subscribe, reset }. Each dispatch aborts the previous in-flight call via a fresh AbortController piped through getAbortablePromise, preserves the last successful data across subsequent running / error states (stale-while-revalidate), and bridges cleanly into useSyncExternalStore-style primitives. Ships with a minor changeset, a new @solana/promises dependency, a lib: ES2024.Promise tsconfig bump for Promise.withResolvers, thorough README docs, and 388 lines of tests.
What I checked
- Supersede semantics:
dispatch#2 synchronously aborts #1's controller and setsrunningbefore awaiting. On #1's late settlement, thesignal.abortedguard correctly skips the stalesetState.previousDatais captured fromstate.databefore therunningtransition, so stale-while-revalidate preserves the genuinely-last-successful value. ✓ - Reset during running:
reset()aborts, sets idle, and the still-pendingdispatchawaits then seessignal.aborted→ skips setState. The "superseded call resolves after reset" test covers this. ✓ - Dependency choices:
@solana/event-target-implstays a devDep (inlined at build time per CLAUDE.md).@solana/promisesis a legitimate runtime dep —getAbortablePromiseis load-bearing for the AbortError rejection contract whenfndoesn't respect the signal. ✓ - Changeset: present, correctly marks
@solana/subscribableasminorfor a new public API. ✓ tsconfigbump toES2024.Promise: required forPromise.withResolversin the tests. No runtime impact since tsup handles compilation. ✓
Things to watch / notes for reviewers
-
Unhandled-rejection ergonomics.
dispatchalways rethrows, including AbortError on supersede/reset. That's a deliberate API choice (callers cantry/catch), but the intended fire-and-forget usage pattern (thesearchStore.dispatch(e.target.value)example in the PR description, and anywheredispatchis wired toonChange/onClick) will produce unhandled AbortError rejections the moment a second dispatch supersedes a first. The tests all.catch(() => {})these, but the README React example doesn't. Worth an explicit call-out in the README's "Things to note" list, or consider swallowing AbortErrors on the returned promise (keeping them onstate.erroris still useful, though you'd want to distinguish abort from user error there too). Flagged inline. -
Broken README anchor.
[isAbortError](../promises#isabortererr)won't resolve — the actual headingisAbortError(err)slugifies to#isaborterrorerr. Flagged inline. -
Listener re-entrancy.
listeners.forEach(l => l())with aSethandles unsubscribe-during-notify correctly, but a listener that synchronously callsdispatch/reset/subscribewill recurse. In practice reactive-UI consumers never do this (and the README's target audience isuseSyncExternalStoreet al.), so I'm not asking for a snapshot-the-listeners change — just noting it. -
currentControllerbookkeeping. After a dispatch settles successfully,currentControllerstill points at the completed controller, so a laterreset()will.abort()an already-aborted-or-completed signal (harmless, just slightly untidy). Not worth changing.
Overall this is a clean, well-tested addition with good API ergonomics and thoughtful docs. My two inline comments are both small.
mcintyre94
force-pushed
the
action-store
branch
from
0cbddad to
d2398a1
Compare
|
I've updated the API so there are now two functions to send the promise. I expect we would expose both in eg
Motivating example: const { sendAsync, isRunning, error } = useSendTransaction();
async function onClick() {
try {
const result = await sendAsync(ix);
navigate(`/tx/${result.signature}`); // needs the awaited value
} catch (err) {
if (isAbortError(err)) return;
toast.error(String(err));
}
}
// needs store state
return <button onClick={onClick} disabled={isRunning}>{error ? 'Retry' : 'Send'}</button>;This matches Tanstack mutations: https://tanstack.com/query/v5/docs/framework/react/guides/mutations#promises This means that UI using Advanced use cases that need to Also stacked the branch on |
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review
Thanks for the fast turnaround. The dispatch / dispatchAsync split is a clean solution to the unhandled-rejection ergonomics — fire-and-forget never rejects (failures surface on state), and imperative callers get a proper promise. API shape, docs, and test coverage all look good.
Still open
- Broken anchor in README —
[isAbortError](../promises#isabortererr)still resolves to a non-existent anchor. GitHub's slug for theisAbortError(err)heading is#isaborterrorerr(it includes theerrparam). I flagged this last round; I don't think it was picked up in the latest push.
New minor nit
dispatchAsynccan resolve with a superseded call's value in a narrow race. Iffnresolves and then the signal aborts before theawaitcontinuation runs,getAbortablePromisehas already won with the value, the!signal.abortedguard correctly skipssetState, but the function stillreturnsresultto the awaiting caller. That contradicts the doc claim thatdispatchAsync"rejects … with anAbortErrorwhen the call is superseded". The easy fix is to checksignal.abortedafter the await and throw anAbortErrorin that branch before returning. Not worth blocking on — the window is tiny and most callers won't notice — but the docs currently overpromise. Flagged inline.
Notes for subsequent reviewers
- The only substantive behaviour change since my last pass is the
dispatch/dispatchAsyncsplit; the core state machine, supersede semantics, and reset behaviour are unchanged and were verified last round. - Tests added coverage for both dispatch variants; I spot-checked the new cases and they look right.
mcintyre94
force-pushed
the
action-store
branch
2 times, most recently
from
c429d73 to
faf218f
Compare
mcintyre94
force-pushed
the
is-abort-error
branch
from
f5f38cb to
15de2e4
Compare
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review
Both open items from the last round are resolved:
- README anchor — now
#isaborterrorerr. ✓ dispatchAsyncsupersede race —signal.abortedis now checked after both the successawaitand in the catch block, throwingsignal.reasonin either case. The catch-branch guard is a nice addition I hadn't explicitly asked for: it prevents a realfnrejection from masking the AbortError when the call is superseded betweenfnrejecting and the continuation running. Two new tests (resolves ... before the continuation runs,rejects ... before the continuation runs) cover both windows. ✓
The CLAUDE.md addition codifying Promise.withResolvers over hand-rolled deferreds is a nice bonus.
Nothing else regressed from my perspective — happy with this now.
lorisleiva
left a comment
lorisleiva
left a comment
There was a problem hiding this comment.
This is super nice!
I like how ReactiveStore and ActionStore follow similar reactive patterns but for different purposes. One is for streaming values whereas the other allows you to dispatch new values yourself.
That being said, I think we could probably improve the way these two concepts sit next to each other. I feel like they are a lot closer than this API makes out. For instance, we should consider breaking the ActionState type into multiple getters like we do with the ReactiveStore.
ReactiveStore |
ActionStore (now) |
ActionStore (proposed) |
|
|---|---|---|---|
TResult |
getState() |
getState().data |
getState() |
| error | getError() |
getState().error |
getError() |
| status | N/A [1] | getState().status |
getStatus() |
Alternatively (if we’re worried about getStatus() and getState() not being fetched automatically) we could do the opposite and make ReactiveStore return a ReactiveState.
Either way, once we unify their APIs and place ReactiveStore next to ActionStore, it becomes clearer that the main difference between the two is that ReactiveStore is readonly whereas ActionStore is a dispatchable version of ReactiveStore. Therefore, we could make that association more obvious by renaming ActionStore to DispatchableReactiveStore.
What do you think?
[1]: Side note, we could even envisage adding a getStatus() function to ReactiveStores for API symmetry even though the the running status wouldn’t be applicable here.
| - **Dev-only code**: Guard with `__DEV__` (e.g. verbose error messages, debug assertions). | ||
| - **Formatting**: ESLint via `@solana/eslint-config-solana`, Prettier via `@solana/prettier-config-solana`. Run `pnpm style:fix` to auto-fix. | ||
| - **All publishable packages share a fixed version** (currently in lockstep). | ||
| - **Deferred promises**: Use `Promise.withResolvers<T>()` instead of hand-rolling a `new Promise((resolve, reject) => ...)` with captured externals. Do not reintroduce a `deferred()` helper — `Promise.withResolvers` already returns `{ promise, resolve, reject }`. |
mcintyre94
force-pushed
the
is-abort-error
branch
from
15de2e4 to
6e1e66f
Compare
mcintyre94
force-pushed
the
action-store
branch
from
faf218f to
0049524
Compare
|
@lorisleiva I like the idea of the two APIs being closer together, but I don't think doing it that way would work as well as it appears. For reactive-store the error case is fatal, rather than being part of the lifecycle. The intended use is (currently) something like: // ErrorBoundary somewhere above
const data = useSyncExternalStore(store.subscribe, () => {
if (store.getError()) throw store.getError();
return store.getState();
});Here Whereas with action-store the intended use case is that you dispatch a function, and it could throw, but that's handled by the store. An error because the user rejected the transaction is recoverable by dispatching again, and the store handles a retried response superseding the previous one. I also think it's important that we have We could expose I think that the action-store design is generally a bit better than reactive-store, because of the more cohesive We likely can't make This would also work for There is also an argument to make This would essentially lead us to a similar outcome of the stores having a more similar shape, but from the opposite direction - bringing the more flexible API here into reactive-store. The state machines are much more similar, and the TLDR: I think there's a stronger case for unifying by bringing the unified state type and retry functionality into reactive-store |
mcintyre94
changed the base branch from
graphite-base/1550
to
rename-reactive-store
mcintyre94
changed the title
createActionStore to @solana/subscribableReactiveActionStore to @solana/subscribable
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review (rename round)
The Reactive-prefix rename is mechanical and clean — ActionStore → ReactiveActionStore, createActionStore → createReactiveActionStore, file/test renamed accordingly, README and barrel updated. Implementation is byte-for-byte the same as last round modulo identifier names; the supersede / reset / dispatchAsync race fixes from prior rounds are preserved. No new code-level concerns.
On Loris's API-unification proposal
For subsequent reviewers — my two cents on Loris's comment, since it's a design call rather than a correctness one:
The two APIs are already heading toward the same shape, just under different names. ReactiveStreamStore already exposes getUnifiedState() returning a single discriminated ReactiveState<T> snapshot, and the per-field getState() / getError() getters on it are explicitly @deprecated in favour of that unified getter (see the README and JSDoc on reactive-stream-store.ts). So the modern, recommended ReactiveStreamStore API is already a single-snapshot getter, exactly like what ReactiveActionStore ships in this PR.
Given that, I'd lean against the first option in Loris's table (splitting ReactiveActionState back into getStatus() / getError() / etc.) — that would walk away from the direction Stream is already headed and re-introduce the snapshot-identity / useSyncExternalStore footgun that getUnifiedState() was added to fix.
The friction Loris is pointing at feels more like naming inconsistency than shape inconsistency:
| Concept | Stream (today) | Action (this PR) |
|---|---|---|
| Discriminated snapshot getter | getUnifiedState() |
getState() |
| Snapshot type | ReactiveState<T> |
ReactiveActionState<TResult> |
IMO the cleanest unification is to align names, not shapes. Two reasonable directions:
- Action adopts Stream's vocabulary — rename to
getUnifiedState()here too, and call the snapshot typeReactiveActionState<…>(already does). Pro: zero churn on the Stream side, which already has deprecation cycles in flight. Con:getUnifiedStateis a slightly awkward name when there's nothing to unify against on the action side. - Stream adopts Action's vocabulary — promote
getState()to be the unified getter on Stream (the deprecated value-onlygetState()is in the way, but it's already deprecated, so a future major can reclaim the name). Pro:getState()reads more naturally and matchesuseSyncExternalStoredocs. Con: adds churn to the Stream deprecation plan.
I'd weakly prefer (2) long-term but (1) is the lower-risk move for this PR if you want to land it without dragging Stream into scope.
On the DispatchableReactiveStore rename — I'd push back gently. "ActionStore" is widely-recognised vocabulary (Redux/Flux/Zustand lineage) and ReactiveActionStore already telegraphs the relationship via the shared Reactive*Store prefix. DispatchableReactiveStore is more architecturally precise but loses the discoverability of "action" as a concept. The shared prefix arguably already does the symmetry work the rename is trying to do.
Not blocking either way — flagging this for discussion before the next push so Loris and Callum can land on a direction without a third rename round.
Notes for subsequent reviewers
- Behaviour and tests are unchanged from the previous approval-shaped pass; no need to re-verify the supersede / reset /
dispatchAsyncrace semantics. - The only outstanding question is the API-shape one above; once that's settled the code itself is in good shape.
mcintyre94
force-pushed
the
rename-reactive-store
branch
from
4319058 to
b1fcac1
Compare
mcintyre94
force-pushed
the
action-store
branch
from
cf0bf8a to
0e977ea
Compare
mcintyre94
force-pushed
the
rename-reactive-store
branch
from
b1fcac1 to
4e1f9d1
Compare
mcintyre94
force-pushed
the
action-store
branch
from
0e977ea to
1a53c39
Compare
mcintyre94
force-pushed
the
rename-reactive-store
branch
from
4e1f9d1 to
09c9c22
Compare
Merge activity
|
#### Problem `ReactiveStore` exposed `getState()` and `getError()` as separate getters with no unified snapshot, which made `useSyncExternalStore` integration 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 `ReactiveStore` closer to the proposed `ActionStore`: #1550. See discussion: #1550 (review) - `ReactiveStore<T>` gains `retry(): void` and `getUnifiedState(): ReactiveState<T>`. `ReactiveState<T>` is a discriminated union over `loading | loaded | error | retrying` with stable snapshot identity across reads. The existing `getState()` and `getError()` getters are preserved but marked `@deprecated`. A future breaking change can make `getState()` the new `getUnifiedState` - New `createReactiveStoreFromDataPublisherFactory` accepts a `() => Promise<DataPublisher>` factory and supports real retry, wiring a fresh inner abort signal for each connection attempt. - The existing `createReactiveStoreFromDataPublisher` is now `@deprecated`. Its `retry()` throws a new `SolanaError` with code `SOLANA_ERROR__SUBSCRIBABLE__RETRY_NOT_SUPPORTED` since a ready-made `DataPublisher` can't be restarted. - `createReactiveStoreWithInitialValueAndSlotTracking` (in `@solana/kit`) 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. - Updated tests for both factories and the slot-tracking store; updated the subscribable README.
mcintyre94
changed the base branch from
rename-reactive-store
to
graphite-base/1550
#### See discussion: #1550 (comment) We decided to rename `ReactiveStore` to `ReactiveStreamStore`, to make space for an orthogonal `ReactiveActionStore`.
Adds `createActionStore` to `@solana/subscribable`: a framework-agnostic state machine that turns any async function into a `{ dispatch, getState, subscribe, reset }` store. The snapshot is a discriminated `ActionState<TResult>` keyed on `status: 'idle' | 'running' | 'success' | 'error'`, so the store always has a defined snapshot and consumers never need to handle `undefined`.
Each `dispatch` creates a fresh `AbortController` and aborts any in-flight predecessor. The superseded call's outcome is dropped unconditionally — stale resolutions and stale failures can never overwrite the newer call's state. The wrapped function receives the `AbortSignal` as its first argument so it can cooperatively cancel network or compute work. This is intended to become the core of a `useAction` React hook but works equally well against Svelte stores, Vue's `shallowRef`, or a vanilla consumer.
Chosen as a sibling of `ReactiveStore<T>` rather than a subtype — the contracts diverge on whether the snapshot is always defined and on how errors surface — and the README calls out the distinction. `dispose()` was intentionally omitted: `reset()` already aborts in-flight work and listeners are garbage-collected when the store is dropped, so a separate disposer would be scope for a future change rather than part of the initial surface.
|
🔎💬 Inkeep AI search and chat service is syncing content for source 'Solana Kit Docs' |
3 participants
Summary of Changes
This PR adds a new
createReactiveActionStoreto@solana/subscribable, which turns any async function into a state machine with subscribe/getState functionality, similar to theReactiveStreamStorewe built for subscriptions.It's designed to be the framework-agnostic reactive-UI-friendly core for all async functions a reactive UI needs. Eg in react this would be the core of hooks like
useConnectWalletanduseSendTransaction.React example:
Functionality:
dispatchto send the promise, fire-and-forget for UI handlers. The state is surfaced through the store, not the resultdispatchAsyncto await the promise and handle any errors. Used for imperative handlers, eg await and then navigate.useSyncExternalStoreand all similar reactive UI primitivesThis is intended to enable a generic
useActionhook in React, which other async functionality likeuseSendTransactionwould build on. Other UI libraries would build a similar primitive. Example (simpified):Why not just use tanstack query etc?
This duplicates some of the functionality of libraries like tanstack query and SWR. The reasons are: