Drop auto-connect from stream stores#1662
Conversation
🦋 Changeset detectedLatest commit: e4b9966 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 (22)
Unchanged files (125)
Total files change +2.83KB +0.54% Final result: ✅ View report in BundleMon website ➡️ |
|
Documentation Preview: https://kit-docs-kjjc6ot88-anza-tech.vercel.app |
mcintyre94
force-pushed
the
reactive-store/no-auto-connect
branch
from
69306ce to
3bccb76
Compare
mcintyre94
changed the title
connect() explicitly
trevor-cortex
left a comment
trevor-cortex
left a comment
There was a problem hiding this comment.
Summary
Drops auto-connect from ReactiveStreamStore so the store starts in idle and only opens its underlying stream when the caller invokes connect(). This mirrors the action-store pattern and sets up per-connection signals in a follow-up PR. retry() is deprecated in favour of connect() (which now both starts the stream and reconnects after an error/load), and a new reset() returns the store to idle without permanently killing it. The deprecated createReactiveStoreFromDataPublisher non-factory variant is removed, along with PendingRpcSubscriptionsRequest.reactive(). Tests across @solana/subscribable, @solana/rpc-subscriptions-spec, and @solana/kit are updated to call connect() explicitly.
State machine summary: idle → (connect) → loading → loaded/error; from loaded/error → (connect) → retrying → loaded/error; from anywhere → (reset) → idle; once abortSignal fires, all connect()s are no-ops.
Things to watch out for
-
Missing
@solana/kitentry in the changeset.packages/kit/src/create-reactive-store-with-initial-value-and-slot-tracking.tsnow starts inidleand requires an explicitconnect()from callers — that's a breaking behavioural change for@solana/kitconsumers, but the changeset only lists@solana/subscribableand@solana/rpc-subscriptions-spec. Kit will still bump via thefixedconfig, but the changelog won't tell consumers why their code stopped firing. Worth adding'@solana/kit': majorand a short migration note (store.connect()after construction, or wrap inuseEffect). -
Doc-comment artifact for
createDataPublisherinFactoryConfig. The JSDoc has two trailing blank*lines and the// FIXMEblock (originally above the now-removeddataPublisherfield) is sandwiched between the JSDoc andcreateDataPublisher: .... The FIXME also still refers to "dataPublisher" by name. See inline. -
loading→retryingwithdata: undefinedwhenconnect()is called during the initial connection. Callingconnect()while still inloading(factory hasn't resolved yet) transitions toretryingwithdata: undefined, even though no prior outcome has occurred. The state-machine doc comment says retrying is entered "after a previous outcome" — there isn't one in this case. This is a minor semantic wrinkle rather than a bug; worth confirming the intent and either tightening the doc, or short-circuitingconnect()to leave the store inloadingwhen called fromloading. See inline.
Notes for subsequent reviewers
SOLANA_ERROR__SUBSCRIBABLE__RETRY_NOT_SUPPORTED(8195000) is now orphaned but correctly retained inpackages/errors/src/codes.tsand theSolanaErrorCodeunion, per the repo's "never remove error codes" policy. The corresponding message inmessages.tsshould also still be present — worth a quick check that it wasn't removed.- The
setStateequality short-circuit (status/data/errortriple compare) prevents spurious subscriber notifications, which is a nice cleanup vs the previous directcurrentState = ...; notify();pattern. currentInnerController?.abort()cleanly tears down the prior connection on everyconnect()andreset(), so listeners on the oldDataPublisherare removed before the new connection wires up — covered by theaborts the prior connection when called again before data arrivestest.- In the slot-tracking variant,
lastUpdateSlotis intentionally preserved acrossconnect()reconnections (so the store never regresses) but reset onreset(). The comment inperformResetcalls this out; worth confirming this is the desired semantics, since a user usingreset()to switch contexts vs to recover the same context might want different behaviours. retry()being kept as a deprecated alias is a reasonable migration ramp, but two semantically-different methods on the same store (connect()always reconnects;retry()only fromerror) may surprise consumers. The deprecation note is good. Worth confirming this is a one-release transitional state rather than a permanent dual-method shape.- Test coverage on the new factory store is solid (idle/connect/reset/retry/abort all covered, including the in-flight-connect-then-reconnect case). The slot-tracking tests are still using the deprecated
retry()rather thanconnect()for the error-recovery scenarios — not wrong (it's the deprecated alias path), but worth double-checking that theconnect()recovery path is exercised at least once in that test file too.
| /** | ||
| * Messages from this channel of `dataPublisher` will be used to update the store's state. | ||
| * An async factory that produces a fresh {@link DataPublisher} each time it is invoked. Called | ||
| * on every {@link ReactiveStreamStore.connect | `connect()`}. Rejections surface as a store | ||
| * error. | ||
| * | ||
| * | ||
| */ | ||
| dataChannelName: string; | ||
| // FIXME: It would be nice to be able to constrain the type of `dataPublisher` to one that | ||
| // definitely supports the `dataChannelName` and `errorChannelName` channels, and | ||
| // furthermore publishes `TData` on the `dataChannelName` channel. This is more difficult | ||
| // than it should be: https://tsplay.dev/NlZelW |
There was a problem hiding this comment.
Two small doc-comment cleanups in this block:
- The JSDoc for
createDataPublisherends with two trailing blank*lines (* error.\n *\n *\n */) — looks like a leftover from the merge. Should be a single closing line. - The
// FIXMEblock (lines 18–21 of this hunk) was originally above the now-removeddataPublisher: DataPublisher;field and refers to "dataPublisher" by name. It's now orphaned mid-declaration, sitting between the JSDoc forcreateDataPublisherand thecreateDataPublisherfield itself. TypeDoc will probably either drop it or attach it to the wrong member. Either move the FIXME above the JSDoc (and reword to mentioncreateDataPublisher/ the factory return type), or delete it if the constraint no longer applies in the factory-only world.
There was a problem hiding this comment.
Still relevant, applies to the return of createDataPublisher. Reworded a bit and moved above the jsdoc. Intentionally not part of the jsdoc though.
| * - `retrying`: a follow-up `connect()` is in progress after a previous outcome. `error` is | ||
| * cleared; `data` is preserved from the previous connection if any. | ||
| */ | ||
| export type ReactiveState<T> = |
There was a problem hiding this comment.
Minor semantic wrinkle: the doc says retrying is "in progress after a previous outcome", but performConnect will transition loading → retrying if connect() is called a second time before the first connection resolves (since the else branch in performConnect fires for any non-idle status, including loading). At that point data is still undefined and no outcome has occurred yet.
Two options worth considering:
- Tighten the doc to acknowledge that
retryingcan also be entered from in-flightloading, or - Short-circuit
performConnectso that re-entering fromloadingstays inloading(only swap the inner controller / factory invocation, no status transition).
Not blocking — just worth picking one explicitly so the state-machine docs and the implementation agree.
There was a problem hiding this comment.
Agree with this, updated so it stays in loading.
| @@ -0,0 +1,22 @@ | |||
| --- | |||
| '@solana/subscribable': major | |||
There was a problem hiding this comment.
createReactiveStoreWithInitialValueAndSlotTracking in @solana/kit is also breaking — it no longer auto-connects, and every caller now needs store.connect() after construction. Kit will bump via the fixed config anyway, but the changelog entry won't surface the migration to consumers.
Suggest adding '@solana/kit': major to the frontmatter and a short paragraph explaining the migration (e.g. "createReactiveStoreWithInitialValueAndSlotTracking no longer fires the RPC request on construction — call store.connect() to start it, or wrap in a useEffect that calls connect() on mount and reset() on cleanup.").
There was a problem hiding this comment.
Fair, added @solana/kit: major
mcintyre94
force-pushed
the
reactive-store/no-auto-connect
branch
from
3bccb76 to
562ec8f
Compare
mcintyre94
force-pushed
the
react/use-request
branch
from
62b80f6 to
cbf3b24
Compare
mcintyre94
force-pushed
the
reactive-store/no-auto-connect
branch
from
562ec8f to
e4b9966
Compare
lorisleiva
left a comment
lorisleiva
left a comment
There was a problem hiding this comment.
Love it! Consistent APIs 🤤
…icitly
`createReactiveStoreFromDataPublisherFactory` (and `createReactiveStoreWithInitialValueAndSlotTracking`) no longer auto-connect on construction. The returned store starts in `status: 'idle'`; the caller calls `store.connect()` to fire the underlying request/subscription, mirroring how `ReactiveActionStore` requires an explicit `dispatch()`. This unifies the lifecycle model across both primitives — stores are state machines that callers orchestrate, not self-starting subscriptions — and lets `useSubscription` adopt the same `useEffect(() => { store.connect(); return () => store.reset(); })` pattern that `useRequest` uses today.
`ReactiveStreamStore<T>` gains two new methods: `connect()` (open or re-open the stream — transitions through `loading` from `idle` or `retrying` from any other status while preserving stale data) and `reset()` (abort the current connection, clearing `data` and `error`, and return to `idle`). The state union grows an `idle` variant. `retry()` becomes a deprecated alias that delegates to `connect()` guarded by `status === 'error'` — preserving its original error-only behaviour for callers who relied on it, but new code should call `connect()` directly.
`createReactiveStoreFromDataPublisher` (the non-factory variant accepting a ready-made `DataPublisher`) is removed. Its only documented consumer was the deprecated `PendingRpcSubscriptionsRequest.reactive()` method, which is also removed in this commit. Both have been deprecated for some time; the cascade removal lands them together rather than ratcheting through two more release cycles. Code that built a store around a singleton publisher should wrap it in `createDataPublisher: () => Promise.resolve(publisher)` and use the factory variant.
`createReactiveStoreWithInitialValueAndSlotTracking` is also lazy: starts in `idle`, requires `connect()` to fire the RPC request and open the subscription. The `lastUpdateSlot` window resets on `reset()` so a fresh `connect()` starts a new slot-tracking lifecycle. Tests in `@solana/subscribable`, `@solana/rpc-subscriptions-spec`, and `@solana/kit` updated accordingly (the `reactive()` test block in `rpc-subscriptions-spec` is removed).
The error code `SOLANA_ERROR__SUBSCRIBABLE__RETRY_NOT_SUPPORTED` is left in the codes table per the never-remove-error-codes rule, but is no longer produced by any code path in this repo.
mcintyre94
force-pushed
the
reactive-store/no-auto-connect
branch
from
e4b9966 to
fb3ce78
Compare
mcintyre94
force-pushed
the
react/use-request
branch
from
cbf3b24 to
2143d96
Compare
3 participants
Summary of Changes
This PR removes auto-connect from
ReactiveStreamStore, such that it starts in an idle state and begins streaming only after.connect()is called. This mirrors the behaviour of the action store, and means that we can add per-connection signals (in a following PR) for subscriptions with a similar API to the action store.We deprecate
.retry(), which was previously only available after an error, in favour of.connect()which now also starts the stream for the first time.The previously deprecated
createReactiveStoreFromDataPublisher, which takes aDataPublisherrather than a() => DataPublisherfunction and therefore can't be called multiple times, is removed.