Refactor createReactiveStoreWithInitialValueAndSlotTracking to use store inputs#1708
Open
mcintyre94 wants to merge 1 commit into
Open
Refactor createReactiveStoreWithInitialValueAndSlotTracking to use store inputs#1708mcintyre94 wants to merge 1 commit into
mcintyre94 wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: d13eb87 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 was referenced May 27, 2026
Member
Author
BundleMonFiles updated (25)
Unchanged files (122)
Total files change +8.77KB +1.66% Final result: ✅ View report in BundleMon website ➡️ |
Contributor
|
Documentation Preview: https://kit-docs-hll1l7wr6-anza-tech.vercel.app |
mcintyre94
force-pushed
the
rpc/sendable-subscribable
branch
from
c71602b to
4399c85
Compare
Member
Author
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
LGTM (commenting because I can't formally approve without explicit repo access).
Summary
Adds two structural duck-types — RpcSendable<TResponse> (just send(options?)) in @solana/rpc-spec and RpcSubscribable<TNotification> (just subscribe(options)) in @solana/rpc-subscriptions-spec — and uses them to loosen the rpcRequest / rpcSubscriptionRequest fields on the configs for createReactiveStoreWithInitialValueAndSlotTracking and createAsyncGeneratorWithInitialValueAndSlotTracking. Both primitives only ever call .send() / .subscribe() on those inputs, so the narrower types are sufficient and existing callers continue to work because PendingRpcRequest / PendingRpcSubscriptionsRequest structurally satisfy the new types.
Mirrors the prior-art pattern from ReactiveActionSource / ReactiveStreamSource, and makes life easier for plugin authors / test mocks that don't want to fabricate a full { reactiveStore, send } object.
What I verified
- The new
RpcSendable.sendsignature is identical toPendingRpcRequest.send(send(options?: RpcSendOptions): Promise<TResponse>), andRpcSubscribable.subscribeis identical toPendingRpcSubscriptionsRequest.subscribe(subscribe(options: RpcSubscribeOptions): Promise<AsyncIterable<TNotification>>) — so structural compatibility actually holds and existing call sites compile unchanged. - The new types flow through to
@solana/kitvia the existingexport *chains (rpc-spec→rpc→kit,rpc-subscriptions-spec→rpc-subscriptions→kit), matching the changeset's documentedimport type { RpcSendable, RpcSubscribable } from '@solana/kit'example. - Both implementation files only consume
.send()/.subscribe()on these inputs, so loosening the type is purely additive at runtime. - Docblock copy on the two config fields was updated from "A pending RPC request" → "A one-shot RPC request" / "An RPC subscription request", which more accurately describes the looser contract.
Changeset
minor across @solana/rpc-spec, @solana/rpc-subscriptions-spec, and @solana/kit looks right — purely additive new exports plus a contravariant input loosening, no breaking change. Fixed-version lockstep handles the umbrella @solana/rpc / @solana/rpc-subscriptions packages automatically.
Notes for subsequent reviewers
- Worth a quick sanity check that no downstream consumer is relying on the specific
PendingRpcRequest/PendingRpcSubscriptionsRequestshape on the config (e.g. calling.reactiveStore()on the passed-in object). A grep overrpcRequest.reactiveStore/rpcSubscriptionRequest.reactiveStorein this repo andkit-pluginsshould be enough. - No tests changed. Given this is type-only and the structural relationship is mechanically obvious, that's fine — but if there's a
__typetests__directory for these primitives, a one-liner asserting that aPending*Requestis assignable to the new config (and that a bare{ send }/{ subscribe }object also is) would lock in the contract cheaply.
mcintyre94
force-pushed
the
rpc/sendable-subscribable
branch
from
4399c85 to
d303d3d
Compare
mcintyre94
force-pushed
the
rpc/sendable-subscribable
branch
from
d303d3d to
eb7e142
Compare
mcintyre94
force-pushed
the
react/depend-on-kit
branch
2 times, most recently
from
b81de7d to
f0abc4e
Compare
mcintyre94
force-pushed
the
rpc/sendable-subscribable
branch
2 times, most recently
from
0153a07 to
e40f088
Compare
mcintyre94
force-pushed
the
react/depend-on-kit
branch
from
f0abc4e to
5dc6620
Compare
mcintyre94
changed the base branch from
react/depend-on-kit
to
graphite-base/1708
mcintyre94
force-pushed
the
graphite-base/1708
branch
from
5dc6620 to
72ffd52
Compare
mcintyre94
force-pushed
the
rpc/sendable-subscribable
branch
from
e40f088 to
1ec107f
Compare
mcintyre94
changed the base branch from
graphite-base/1708
to
react/depend-on-kit
This was referenced May 28, 2026
mcintyre94
force-pushed
the
rpc/sendable-subscribable
branch
4 times, most recently
from
4ba1928 to
b50eb7f
Compare
mcintyre94
changed the title
RpcSendable / RpcSubscribable duck-types and loosen reactive-store config
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Re-review (rewritten PR)
LGTM on the new shape — this is the right pivot in response to Loris's comment, and the design holds together cleanly. Commenting because I can't formally approve.
What changed since last review
The PR no longer adds RpcSendable / RpcSubscribable to rpc-spec / rpc-subscriptions-spec. Instead createReactiveStoreWithInitialValueAndSlotTracking is rewritten to consume two reactive sources — ReactiveActionSource for the one-shot fetch and ReactiveStreamSource for the ongoing stream — calling reactiveStore() on each per connection window and forwarding their state into the unified store. The previous rpcRequest / rpcSubscriptionRequest (+ matching mappers) config fields are renamed to initialValueSource / streamSource (+ initialValueMapper / streamValueMapper). Behaviour is preserved.
Net effect: this primitive now lives entirely in the reactive-store universe (matching the React hooks), and send() / subscribe() are no longer called directly anywhere in the slot-tracking helper.
What I verified
PendingRpcRequest/PendingRpcSubscriptionsRequeststill slot in unchanged. Both already exposereactiveStore(), so the changeset's claim that callers can still passrpc.getBalance(addr)/rpcSubscriptions.accountNotifications(addr)directly holds. The example app diff confirms this — the only call-site change is the field renames (plus an explicitwithSignal(...).connect(), which is the new required entry point).- Lifecycle plumbing is sound.
performConnectbuilds fresh inner action + stream stores on every (re)connect, subscribes to both, registers a singleinnerSignal'abort'listener that unsubscribes both, and then kicks them viawithSignal(signal).connect()/.dispatch()wheresignalis the composed inner+caller signal. On supersede/reset/caller-abort, the inner controller fires, the unsubscribe listener runs, and the inner stores are orphaned with their own connections cancelled by the same signal — no listener leak, no double state writes (the existinghandleErrorsignal.abortedguard +currentState.status === 'error'guard cover the late-error race). - Slot tracking centralised.
handleSlottedValuenow applies theslot < lastUpdateSlotskip to both sources symmetrically (previously the RPC path and subscription path each had their own copy), andlastUpdateSlotstill resets inperformReset. The “older RPC arrives after newer subscription” / “older subscription arrives after newer RPC” tests still pass against this unified path. - Inner-store API choice is correct. The action subscriber reads
actionStore.getState()(which returns the discriminatedReactiveActionState), and the stream subscriber readsstreamStore.getUnifiedState()(the non-deprecated stream API). The deprecatedgetState()/getError()onReactiveStreamStorearen't used internally. - Tests rewritten against real inner stores. The mocks now back
initialValueSourcewith a realcreateReactiveActionStoreover a controllable promise factory, andstreamSourcewith a realcreateReactiveStoreFromDataPublisherFactoryover a mockDataPublisher. That's a strictly better test than the previous hand-rolledsend/subscribemocks — it exercises the actual contract instead of asserting against a fake. expect.assertions(n)only on async tests, fake timers +await jest.runAllTimersAsync()for flushing,Promise.withResolvers— all matchesCLAUDE.md's testing conventions.- Changeset bump.
majorfor@solana/kitis right:rpcRequest/rpcSubscriptionRequest(and their matching mappers) are gone from the config, so any existing caller breaks at compile time. Fixed-version lockstep will propagate the bump to the rest of the publishable packages automatically — no need to list them individually. - README + example app + JSDoc all consistently use the new field names and the explicit
withSignal(...).connect()entry point. The example app'swithSignal(abortController.signal).connect()line is the one behavioural call-site change worth flagging to next reviewers — previously the abort signal was implicitly carried by thePendingRpc*Request; now it has to be threaded explicitly.
Minor nits (non-blocking)
- The block comment above
const actionStore = initialValueSource.reactiveStore()(increate-reactive-store-with-initial-value-and-slot-tracking.ts, around the new L188–L191) still says “Both stores only ever exposesend()/subscribe()-equivalent work through their reactive contract.” That's a leftover from the prior approach and reads as if the helper is still wrappingsend/subscribedirectly. Could be simplified to something like: “Build fresh inner stores for this connection window and forward their state into the unified store. Drive both with the composed signal so they tear down when this connection window does.” Inline comment below. - The cleanup comment
// Note: don't call reset here, causes a race with the abort reasonis slightly cryptic — worth a one-line elaboration so a future reader doesn't try to “fix” it by addingreset()calls and break the abort-reason propagation. Inline comment below. - Test file imports
ReactiveActionSource/ReactiveStreamSourceas values rather thanimport type. They're only used in type positions in the test file, soimport typewould be marginally cleaner — butverbatimModuleSyntaxisn't on, so this is purely stylistic. Non-blocking.
Notes for subsequent reviewers
- Worth thinking about: there's now no public test that exercises a
PendingRpcRequest/PendingRpcSubscriptionsRequestactually being passed in as the source. The test suite uses syntheticReactiveActionSource/ReactiveStreamSourceobjects. The structural relationship is mechanical (both pending-request types exposereactiveStore(): ReactiveActionStore<[], T>/reactiveStore(): ReactiveStreamStore<T>), but a one-liner__typetests__assertingPendingRpcRequest<SolanaRpcResponse<T>> satisfies ReactiveActionSource<SolanaRpcResponse<T>>(and the subscription equivalent) on the config type would lock the duck-type contract in cheaply and survive any future signature drift onPendingRpc*Request. - Behavioural call-site delta to call out in release notes: prior callers that relied on the implicit caller-signal propagation through
PendingRpc*Requestnow need an explicitwithSignal(signal).connect(). The example app diff shows exactly this. The changeset entry is clear about the API rename but doesn't explicitly call out the connect-now-required-to-fire bit — might be worth a sentence.
| // something newer before leaving `loading`. | ||
| function handleSlottedValue({ context: { slot }, value }: SolanaRpcResponse<TItem>) { | ||
| if (signal.aborted) return; | ||
| if (slot < lastUpdateSlot) return; |
There was a problem hiding this comment.
Nit: this comment is a leftover from the previous send() / subscribe()-based implementation and reads as if the helper is still wrapping those calls directly. Now that we're consuming the reactive-store contract instead, something like:
Build fresh inner stores for this connection window and forward their state into the unified store. Drive both with the composed signal so they tear down when this connection window does.
would more accurately describe what the block is doing. Non-blocking.
(Anchoring on the last line of the comment block — confirm the new wording lines up with whatever you settle on.)
| if (state.status === 'loaded') { | ||
| const { context, value } = state.data; | ||
| handleSlottedValue({ context, value: streamValueMapper(value) }); | ||
| } else if (state.status === 'error') { |
There was a problem hiding this comment.
Nit: worth a one-line elaboration here so a future reader doesn't try to “improve” this by calling actionStore.reset() / streamStore.reset() and accidentally clobber the caller-signal abort reason. Something like:
Don't call
reset()on the inner stores — that would set them toidlesynchronously, which races the abort-reason error transition already in flight on the composed signal and can drop the caller's abort reason on the floor.
(or whatever the actual failure mode you saw was — that's my best guess from reading the surrounding code). Non-blocking.
Member
Author
There was a problem hiding this comment.
I think the comment this is referring to is: "// Note: don't call reset here, causes a race with the abort reason"
I think this is fine and enough to tell someone not to call reset, we don't need to detail how the race happens
mcintyre94
force-pushed
the
rpc/sendable-subscribable
branch
from
b50eb7f to
3cc903c
Compare
This was referenced May 30, 2026
mcintyre94
force-pushed
the
rpc/sendable-subscribable
branch
from
3cc903c to
7c048b2
Compare
mcintyre94
force-pushed
the
react/depend-on-kit
branch
from
fd51da0 to
6c7bd1d
Compare
mcintyre94
force-pushed
the
rpc/sendable-subscribable
branch
from
7c048b2 to
e8f4058
Compare
mcintyre94
force-pushed
the
react/depend-on-kit
branch
from
8dc7e3a to
e63a414
Compare
mcintyre94
force-pushed
the
rpc/sendable-subscribable
branch
from
e8f4058 to
7333125
Compare
mcintyre94
force-pushed
the
react/depend-on-kit
branch
from
e63a414 to
183fcb1
Compare
mcintyre94
force-pushed
the
rpc/sendable-subscribable
branch
from
7333125 to
06be15d
Compare
mcintyre94
force-pushed
the
react/depend-on-kit
branch
from
183fcb1 to
bd5965a
Compare
mcintyre94
force-pushed
the
rpc/sendable-subscribable
branch
from
06be15d to
12d6126
Compare
mcintyre94
force-pushed
the
react/depend-on-kit
branch
from
bd5965a to
e870691
Compare
mcintyre94
force-pushed
the
rpc/sendable-subscribable
branch
from
12d6126 to
ec7a400
Compare
mcintyre94
added
the
do-not-close
inputs Now takes a `ReactiveActionStore` instead of `PendingRpcRequest`, and `ReactiveStreamStore` instead of `PendingRpcSubscriptionRequest`
mcintyre94
force-pushed
the
react/depend-on-kit
branch
from
e870691 to
ef9e7b1
Compare
mcintyre94
force-pushed
the
rpc/sendable-subscribable
branch
from
ec7a400 to
d13eb87
Compare
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.
Note: this PR has been rewritten, first review comments refer to an old version
Problem
createReactiveStoreWithInitialValueAndSlotTrackingcurrently takes as input aPendingRpcRequestand aPendingRpcSubscriptionsRequest. Internally it is using thesend()andsubscribe()functions, and not thereactiveStore()that is used by the react hooks.Summary of Changes
createReactiveStoreWithInitialValueAndSlotTrackingis refactored to take aReactiveActionStoreand aReactiveStreamStore, and its internals are rewritten to produce its store from these inputs.Its config now has the shape:
Its behaviour is unchanged - it combines the initial value and the stream and always provides the latest available data (based on slot) from either source
The following PR adds a react hook that exposes
createReactiveStoreWithInitialValueAndSlotTracking, and it will now share the same input types with the other react hooks.