Skip to content

fix: complete direct subscribers of preloaded observables on error - #852

Open
tyler-reitz wants to merge 3 commits into
FirebaseExtended:mainfrom
tyler-reitz:fix/preload-error-complete
Open

tyler-reitz wants to merge 3 commits into
FirebaseExtended:mainfrom
tyler-reitz:fix/preload-error-complete

Conversation

@tyler-reitz

@tyler-reitz tyler-reitz commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Direct subscribers of preloadFirestoreDoc and preloadObservable results complete again when the source errors, as in 4.3.0.

Since #839 (unreleased), they received next(undefined) and the stream never completed: firstValueFrom resolved undefined and lastValueFrom never settled.

Changes

  • SuspenseSubject._subscribe completes the subscriber on a source error.
  • New _subscribeToStore(onStoreChange), used only by useObservable. 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.
  • Only a _subscribeToStore subscription 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 on main, 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.
  • Both share the existing subscribe logic, moved to a private _attach.
  • _subscribeToStore appears in the emitted SuspenseSubject.d.ts (the return type of preloadObservable), next to _subscribe; the underscore marks it internal.

Neither version calls a direct subscriber's error callback. 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:

When the source errors, direct subscribers of preloadFirestoreDoc and preloadObservable results receive undefined and the stream never completes...

Compared with main: a direct subscriber's manual unsubscribe() no longer evicts an errored entry a Suspense hook has read. 4.3.0 did not evict it either.

Testing

Test Red when
A direct subscriber, and one that arrives after the error, get complete only; lastValueFrom rejects with EmptyError on main (next:undefined)
A direct subscriber gets an emitted value, then complete on a later error _subscribe drops values
A component subscribing directly in its render body and reading the same id with Suspense reaches the error boundary after 1 source subscription a direct subscriber's teardown evicts again
Existing Suspense error-eviction test useObservable goes back through the completing path
  • The existing resubscribe-loop guard no longer goes red on that last mutation, since a direct teardown no longer evicts (checked on 4738b10).
  • Full emulator suite on React 18.2 and 19.3.1, tsc on both configs: clean apart from the known local RTDB double check emulator timeout.
  • No docs regen needed: docs/reference has no links into SuspenseSubject.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.

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 armando-navarro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 _attach runs 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.
    • main and 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.
    • main and 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 _subscribeToStore subscription 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 main for the notes: there, a direct subscriber's manual unsubscribe() 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 the next callback in _subscribe can 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 the describe title, refer to the if (this._error) branch in _subscribe's complete handler, 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 armando-navarro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.tsx say a completed hook with initialData resubscribes in a loop.
  • I wrote that, and the same commit made it untrue.
  • With useObservable back on subscribe(), 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants