Repository navigation
fix: complete direct subscribers of preloaded observables on error - #852
tyler-reitz wants to merge 3 commits into
Conversation
Since FirebaseExtended#839, a direct subscriber of a preloadFirestoreDoc or preloadObservable result received next(undefined) on a source error and never completed, so firstValueFrom resolved undefined and lastValueFrom never settled. 4.3.0 completed. useObservable now subscribes through _subscribeToStore, which notifies without completing, and every other subscriber completes on error again.
armando-navarro
left a comment
There was a problem hiding this comment.
Hey Tyler, the direct-subscriber events now match 4.3.0 for me. One thing I'd want changed before approving: ending the stream for a direct subscriber also evicts the errored entry, and with a Suspense reader on the same id that turns into repeated reads.
1. Completing a direct subscriber evicts the errored entry (blocking)
What I found on this head:
subscriber.complete()unsubscribes the subscriber, so the teardown in_attachruns when the source errors.- That teardown evicts an errored entry once the subscriber count is zero.
- A Suspense hook suspended on the same id then retries onto a new entry and subscribes to the source again.
- With one direct subscriber created outside React:
- This head makes 2 source subscriptions before the boundary shows the error.
mainand 4.3.0 make 1.
- With a direct subscription made in the render body of the suspended component, the retry repeats:
- This head made about 60 source subscriptions in 3 seconds with a 50ms source.
- The boundary had still not shown the error.
mainand 4.3.0 show it after 1 subscription.
- A late direct
.subscribe()on an errored entry that a Suspense hook has read evicts it as well, so the next mount reads again. #839's notes say that entry stays cached until reload.
What fixed it for me:
- Only the
_subscribeToStoresubscription evicts an errored entry when it unsubscribes. A direct subscriber's teardown leaves the entry alone. - With that, all three cases above make 1 source subscription, and the full emulator suite passes.
- One difference from
mainfor the notes: there, a direct subscriber's manualunsubscribe()evicted an errored Suspense entry. With this change it does not, which is what 4.3.0 did.
2. The new test checks events only (optional, the first goes with the fix)
Two cases I'd add:
- A component that subscribes directly in its render body and reads the same id with Suspense reaches the error boundary after 1 source subscription. It fails on this head and passes with the change above.
- A source that emits a value and then errors gives a direct subscriber the value, then
complete. Today thenextcallback in_subscribecan be replaced with a no-op and every test still passes.
3. Comments in the test file describe the removed branch (optional)
- Lines 8 to 11 and line 58 of
test/suspenseSubject-error-branch.test.tsx, and thedescribetitle, refer to theif (this._error)branch in_subscribe'scompletehandler, which this PR removes.
Section 1 is what I'd need for an approve. I have the change and both tests written and can push them as a branch if that helps.
If you expect direct subscribers to be used in a way that makes the render-body case impossible, tell me and I'll look again.
Completing a direct subscriber on a source error unsubscribes it, and its teardown evicted the errored entry. A Suspense render suspended on the same id then retried onto a new entry and read the source again, without end when the subscription was made in that component's render body. 4.3.0 never evicted here. Only the useObservable subscription now evicts an errored entry on unsubscribe. Adds tests for the render-body case and for a value followed by an error, and updates the test file's comments.
armando-navarro
left a comment
There was a problem hiding this comment.
Approving: the head is the commit I pushed, and your new description matches what I measured, including the note that the resubscribe-loop test no longer fails when the hook goes back through subscribe().
1. My header comment in the test file is out of date (optional)
- Lines 8 to 10 of
test/suspenseSubject-error-branch.test.tsxsay a completed hook withinitialDataresubscribes in a loop. - I wrote that, and the same commit made it untrue.
- With
useObservableback onsubscribe(), the only test that fails is "evicts an errored entry when its last Suspense subscriber unmounts, so a remount reads fresh". - What goes wrong now: the hook is unsubscribed while still mounted, so its unmount no longer evicts the errored entry.
2. The second Changes bullet gives the same old reason (optional)
- It says completing would evict the errored entry and make the next render re-fetch.
- Your Testing note already says a direct teardown no longer evicts.
Neither needs another round from me.
Direct subscribers of
preloadFirestoreDocandpreloadObservableresults complete again when the source errors, as in 4.3.0.Since #839 (unreleased), they received
next(undefined)and the stream never completed:firstValueFromresolvedundefinedandlastValueFromnever settled.Changes
SuspenseSubject._subscribecompletes the subscriber on a source error._subscribeToStore(onStoreChange), used only byuseObservable. It notifies on every change, the error included, and never completes. Completing would unsubscribe the hook while it is still mounted, so its unmount would no longer evict the errored entry._subscribeToStoresubscription evicts an errored entry when it unsubscribes (fix by @armando-navarro). A direct subscriber is unsubscribed when it completes on error, and evicting then sent a suspended reader on the same id to a fresh entry and a second source subscription (2 vs 1 onmain, with the direct subscriber outside React). Armando measured a direct subscription in the render body repeating about 60 times in 3s without reaching the error boundary._attach._subscribeToStoreappears in the emittedSuspenseSubject.d.ts(the return type ofpreloadObservable), next to_subscribe; the underscore marks it internal.Neither version calls a direct subscriber's
errorcallback. Delivering the error there would be a behavior change and is left for v5.Release notes
Drop this line from #839's notes; it no longer applies:
Compared with
main: a direct subscriber's manualunsubscribe()no longer evicts an errored entry a Suspense hook has read. 4.3.0 did not evict it either.Testing
completeonly;lastValueFromrejects withEmptyErrormain(next:undefined)completeon a later error_subscribedrops valuesuseObservablegoes back through the completing path4738b10).tscon both configs: clean apart from the known local RTDBdouble checkemulator timeout.docs/referencehas no links intoSuspenseSubject.ts.Side effect: a mounted hook on a stream that completes normally now stays subscribed until unmount instead of auto-unsubscribing. The subscriber count only drives error eviction, which a completed stream never reaches.