fix: surface observable errors via status instead of re-throwing#735
fix: surface observable errors via status instead of re-throwing#735tyler-reitz wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request bumps the package version to 4.3.0 and modifies useObservable to surface errors via the status field instead of throwing them. Feedback highlights that completely removing the error throw breaks the React Suspense contract, where errors should be caught by an ErrorBoundary. It is recommended to conditionally throw errors when suspense is enabled and to update the tests accordingly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
f6fc807 to
6c015ca
Compare
armando-navarro
left a comment
There was a problem hiding this comment.
Verified this end-to-end locally — the fix itself is right.
Requesting changes on three things, all in service of this being the breaking change the description says it is:
Documentation
- This changes default error behavior for every hook, and right now the migration story exists only in this PR description (which becomes a squash-commit message).
- At minimum: a JSDoc update on
useObservable/ObservableStatusdescribing the mode split (which flows intodocs/reference— will need a docs regen) - An error-handling example in
docs/use.md(its current storage example checks onlyloading, so after this merges it silently renders<img src={undefined}>on error — the docs would be teaching the silent-failure pattern) - An entry in
docs/upgrade-guide.md.
Type contract
ObservableStatusErrordeclaresisComplete: true, but runtime isfalsein every error case I tested — the@ts-expect-errorinSuspenseSubject._updateImmutableStatusacknowledges it.- This was latent while errors always threw; now that
status === 'error'is the recommended branch, TypeScript narrows consumers into trusting a value that's wrong at runtime. Either make itisComplete: booleanor set the runtime flag on error.
Tests for what this PR actually promises
- The single added test uses a synthetic
throwErrorwith explicitsuspense: false. - Missing:
- the marquee #535 path (
useStorageDownloadURL, nonexistent object, default mode, emulator); - bare default (
useObservable(id, obs$)with no suspense option — the headline case); theFirebaseAppProvider suspense={true}context path still throwing (distinct code branch, no error test); - and the late-error contract (emit-then-error leaves
status: 'error'with staledatastill readable — real behavior, currently undocumented and untested;- if it's intended, a test locks it in; if not, that's a bug to fix). I ran all of these locally and they behave as described — happy to share the exact tests.
- the marquee #535 path (
Two decisions you might want stated explicitly in the PR (not code changes):
- Semver. The description says breaking; the bump says 4.3.0 (minor). Repo precedent for breaking was a major (3→4, the Firebase v9 change).
- There's a fair argument this is a spec-violation fix (
status: 'error'was always documented and typed, just unreachable) — but that call should be made on purpose, not by the version number in the diff. - Note the committed bump only affects
expcanary numbering either way; tags drive real releases.
- There's a fair argument this is a spec-violation fix (
- Error recovery. Once a mounted cache entry errors, nothing un-errors it (the cache reset timer dies on first subscription).
- That's pre-existing — but this PR directs users to build
status === 'error'UI around a state with no retry path. - Fine to punt to a follow-up issue; it should be a stated decision.
- That's pre-existing — but this PR directs users to build
Non-blocking notes
StorageImagenow swallows errors silently (placeholder forever,errornever read) where it used to crash — worth either readingerror(log or render slot) or an explicit "known limitation" note;- the new test sits in the
Suspense Modedescribe block but tests non-suspense (move it, and don't assertisComplete: trueon error — it'sfalse); - #540's thread proposed an explicit opt-in flag (
throwErrorInObservable) rather than coupling to suspense mode — your coupling is defensible and simpler, just worth acknowledging the divergence in the thread when this closes it.
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "version": "4.2.3", | |||
| "version": "4.3.0", | |||
There was a problem hiding this comment.
See "Semver" in my review — the description says breaking, this says minor. Worth an explicit 4.3.0-vs-5.0.0 decision (this number only drives exp canary versions either way).
Removes the unconditional re-throw in useObservable that made status: 'error' unreachable. Errors are now returned in ObservableStatus so consumers can handle them locally via status checks. Users who want Error Boundary behavior can opt in by throwing the error themselves. Fixes FirebaseExtended#535, fixes FirebaseExtended#540
Preserves Error Boundary behavior for suspense mode while allowing non-suspense consumers to handle errors locally via status === 'error'.
- Fix ObservableStatusError.isComplete type from `true` to `boolean` (runtime is false when error fires before complete) - Add JSDoc to useObservable documenting the suspense/non-suspense error handling split - Move error test from Suspense Mode block to Non-Suspense Mode block - Add test for bare default mode (no suspense option) surfacing errors via status - Add test for late-error contract (stale data remains readable after error) - Update docs/use.md storage example to handle status === 'error' - Add v4.2 to v4.3 upgrade guide section
5d4e6d7 to
13bfb96
Compare
Problem
useObservableunconditionally re-throws errors from the underlying observable, makingstatus: 'error'unreachable in practice. Users in non-suspense mode who want to handle errors locally have no way to do so:The original code included a TODO from the author questioning whether this behavior was correct.
Fix
Error handling now matches the mode the consumer opted into:
suspense: true): errors still throw, so React Error Boundaries catch them as expected. This preserves the idiomatic Suspense contract.suspense: false): errors are returned viastatus: 'error'so consumers can handle them locally.Breaking change
This affects the default usage pattern. Non-suspense mode is the default — if you don't pass
suspense: trueor wrap your app in aSuspenseEnabledContext, you are in non-suspense mode.Before 4.3.0, errors from any reactfire hook would throw unconditionally, regardless of mode. After 4.3.0:
status, no change needed.Open decisions (need Jeff input)
Semver: This PR is bumped as 4.3.0 (minor). Repo precedent for breaking changes was a major bump (v3 to v4 for the Firebase v9 migration). There is a reasonable argument this is a spec-violation fix (
status: 'error'was always documented and typed, just unreachable in practice) rather than a new behavior, which would support staying at a minor. Either way, that call should be made deliberately. Tagging Jeff to decide before merge.Error recovery: Once a mounted cache entry errors, there is no retry path. The 30s reset timer is cancelled on first subscription, and errored observables do not re-subscribe. This is pre-existing behavior, not introduced by this PR. Surfacing it here because this PR directs users to build
status === 'error'UI around a state with no retry path. Punting a proper retry mechanism to a follow-up issue; noting it here so the decision is explicit.Fixes #535, fixes #540