Collapse retrying into loading with stale-while-revalidate semantics#1677
Draft
mcintyre94 wants to merge 1 commit into
Draft
Collapse retrying into loading with stale-while-revalidate semantics#1677mcintyre94 wants to merge 1 commit into
retrying into loading with stale-while-revalidate semantics#1677mcintyre94 wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 2fd75aa 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 |
Member
Author
mcintyre94
changed the title
loading and retrying into a single loading statusretrying into loading with stale-while-revalidate semantics
BundleMonFiles updated (22)
Unchanged files (125)
Total files change +5.63KB +1.07% 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.
Collapses retrying into loading on ReactiveStreamStore, mirroring the action store's running, and widens loading to carry data: T | undefined + error: unknown so the prior outcome is preserved across reconnects (SWR). Reasonable shape change — the implementation in both stores becomes uniformly setState({ data: currentState.data, error: currentState.error, status: 'loading' }) and the existing setState identity guard naturally no-ops the loading → loading re-entry (already covered by the pre-existing 'does not notify subscribers on the loading → loading re-entry' test).
packages/subscribable/README.md is half-migrated and now self-contradictory. The PR only made minimal textual tweaks here, leaving stale retrying references that disagree with the implementation. Three spots:
- The
ReactiveState<T>snippet in the README still lists aretryingvariant (lines 38–42 of the new README). The diff just changederror: undefined→error: unknownon that line; the line should be removed outright, sinceretryingis no longer part of the type. As-is, the README contradicts the source. - The paragraph just below (line 47) still says "transitions through
retryingwhile preserving the last known value and the last error". - Further down (around line 135), the
createReactiveStoreFromDataPublisherFactory"Things to note" bullet wasn't touched at all and still reads: "Fromidle, the store transitions throughloading; from any other status, throughretryingwhile preserving the last known value." Same problem.
Inline comments on the first two. #3 isn't part of the diff so flagging it here.
Things to watch out for / verify:
- Migration heuristic in the changeset (
status === 'loading' && data !== undefined) misses the case where a retry was kicked off fromerrorbut noloadedhad ever occurred before that error — thendata === undefinedand the loading is indistinguishable from a first-load from idle. The changeset already acknowledges this loosely ("or juststatus === 'loading'if you don't need to distinguish"), so this is fine, but worth keeping in mind for downstream callers that genuinely need the first-load-vs-reconnect distinction — the new model doesn't preserve it. - Test coverage looks good: kit's
error → loading (preserves data + error), subscribable'serror → loadingandloaded → loading, and theloading → loadingno-notify case are all covered. The notify-on-transition tests still assert the right thing (the status discriminator changes, so subscribers still fire). - Behavioural diff worth keeping in the back of the head for downstream consumers: previously, a
connect()fromerrorclearederrorimmediately on transition, so any UI gated onerror !== undefinedwould clear at that moment. Nowerrorsurvives across the transition until the nextloaded(or a newerror) replaces it. This is the intended SWR semantic and matches the changeset, just want it surfaced for review.
Notes for subsequent reviewers:
- Pre-existing staleness elsewhere in the README that's outside the scope of this PR but worth a follow-up: the bottom
createReactiveStoreFromDataPublisherFactorysnippet still shows anabortSignal:property on the config and the bullet about "Triggering the caller'sabortSignaldisconnects the store permanently" — neither matches the currentwithSignal()-per-connect API. Not blocking this PR, but worth knowing while the file is open. - No other call sites in the repo reference
'retrying'as far as I can see; the type-system removal of the variant would surface any stragglers at compile time.
| | { data: T; error: undefined; status: 'loaded' } | ||
| | { data: T | undefined; error: unknown; status: 'error' } | ||
| | { data: T | undefined; error: undefined; status: 'retrying' } | ||
| | { data: T | undefined; error: unknown; status: 'retrying' } |
There was a problem hiding this comment.
This line should be removed, not edited — retrying is no longer a variant of ReactiveState<T>. The PR only flipped error: undefined → error: unknown here, which leaves the README claiming a variant that doesn't exist in reactive-stream-store.ts.
The replacement variant (loading widened to data: T | undefined; error: unknown) needs to be reflected on the existing loading line a few rows up too.
| > Also exported as `ReactiveStore<T>` for backwards compatibility. That alias is deprecated and will be removed in a future major release. | ||
|
|
||
| The store starts in `status: 'idle'`. Call `connect()` to open the underlying stream; the store will transition through `loading` → `loaded` (or `error`). A subsequent `connect()` after `loaded` or `error` transitions through `retrying` while preserving the last known value. Call `reset()` to tear down the connection and return to `idle` without permanently killing the store. | ||
| The store starts in `status: 'idle'`. Call `connect()` to open the underlying stream; the store will transition through `loading` → `loaded` (or `error`). A subsequent `connect()` after `loaded` or `error` transitions through `retrying` while preserving the last known value and the last error (stale-while-revalidate). A subsequent `loaded` clears the error; a subsequent `error` replaces it. Call `reset()` to tear down the connection and return to `idle` without permanently killing the store. |
There was a problem hiding this comment.
Still mentions retrying, which no longer exists. Needs to be rewritten along the lines of the source TSDoc — e.g. "A subsequent connect() after loaded or error transitions back through loading while preserving the last known data and error (stale-while-revalidate)...". The A subsequent loaded clears the error; a subsequent error replaces it. sentence afterwards is correct and can stay as-is.
Contributor
|
Documentation Preview: https://kit-docs-haq8ian6e-anza-tech.vercel.app |
Mirrors the action store's `running` (which already merged "first call vs subsequent call"). `data` and `error` are preserved through `loading` for stale-while-revalidate UX.
`ReactiveState<T>` drops the `retrying` variant. `loading` widens from `{ data: undefined, error: undefined }` to `{ data: T | undefined, error: unknown }`. Both `createReactiveStoreFromDataPublisherFactory` and `createReactiveStoreWithInitialValueAndSlotTracking` transition every `connect()` through `loading`, preserving `currentState.data` and `currentState.error`.
Consumers that need to distinguish first-load from reconnect inspect `data === undefined` instead of relying on a separate `retrying` status. Bumps `@solana/subscribable` and `@solana/kit` major.
Tests in both packages updated: `retrying`-state assertions now assert `loading` carrying stale data + error. Downstream `useSubscription` (next commit) drops `retrying` from `SubscriptionResult.status` and passes data + error through the `loading` case.
mcintyre94
force-pushed
the
reactive-store/preserve-retrying-error
branch
from
efde42d to
2fd75aa
Compare
Member
Author
|
Oops missed some changes to the readme. Please re-review @trevor-cortex |
|
Re-reviewed the README diff — all three stale
Nice bonus: the pre-existing Nothing else to flag. LGTM. 👍 |
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.
Summary of Changes
This PR modifies the states for the stream-store.
The
retryingstate is removed, and we now useloadingfor both first connects and subsequent ones. This matcheshow we structure the action store, and minimises states.Additionally we preserve the last error (in addition to data) in the
loadingstate, to improve SWR. Again this matches a change made to the action store.