Skip to content

fix: defer eager requests in storage, auth, and functions hooks#757

Merged
tyler-reitz merged 4 commits into
FirebaseExtended:mainfrom
tyler-reitz:fix/defer-eager-requests
Jul 24, 2026
Merged

fix: defer eager requests in storage, auth, and functions hooks#757
tyler-reitz merged 4 commits into
FirebaseExtended:mainfrom
tyler-reitz:fix/defer-eager-requests

Conversation

@tyler-reitz

Copy link
Copy Markdown
Contributor

What

useStorageDownloadURL, useIdTokenResult, and useCallableFunctionResponse each constructed their request in the render body, so the underlying call ran on every render, firing a discarded request each time (and, for a missing storage object, an unhandled promise rejection). This wraps each in rxjs defer so the request is created lazily on subscription. Since useObservable subscribes once per observableId, the request now runs once instead of once per render.

  • useStorageDownloadURL: defer(() => getDownloadURL(ref))
  • useIdTokenResult: defer(() => from(user.getIdTokenResult(forceRefresh)))
  • useCallableFunctionResponse: defer(() => obsFactory(options?.data))

Closes #743, closes #744.

Why a separate PR

Split out of #735 at Jeff's request: the defer fix is a non-breaking bugfix that can ship in a v4 patch, independent of #735's (breaking) error-surface change, which is now slated for v5.

Testing

Adds a regression test for useIdTokenResult using an injected user whose getIdTokenResult is a spy: it is called once across re-renders with the wrapper, and 4 times without it (verified locally). The three hooks share the same one-line defer pattern through useObservable, so this pins the mechanism. Happy to add module-mock call-count tests for the storage and functions hooks too if reviewers would like per-hook coverage.

useStorageDownloadURL, useIdTokenResult, and useCallableFunctionResponse
built their request in the render body, so a discarded request fired on
every render (and, for a missing storage object, an unhandled promise
rejection). Wrap each in rxjs `defer` so the request is created lazily on
subscription. useObservable subscribes once per observableId, so the
request now runs once instead of once per render.

Adds a regression test for useIdTokenResult confirming the deferred
factory runs exactly once across re-renders (4 calls without the wrapper).

Closes FirebaseExtended#743, FirebaseExtended#744.
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@armando-navarro armando-navarro left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice, clean fix. I verified all three hooks on current main (post-4.2.5): each now creates its request lazily on subscription instead of in the render body, and I confirmed the change is behavior-preserving apart from removing the per-render repeat.

What I verified

  • The per-render waste is real and the fix removes it: with the new test, getIdTokenResult is called once across a render plus two re-renders; reverting just the defer wrapper makes it fail with four calls. So the test genuinely pins the fix.
  • defer is safe under this architecture: SuspenseSubject subscribes eagerly in its constructor and the source ends in shareReplay(1), so the request still fires at the same moment as before and runs exactly once. Cache identity, the suspense firstEmission path, and error surfacing are all unchanged.
  • The storage unhandled-rejection fix is real rather than relocated: previously each non-first render created a fresh rejecting promise with no subscriber; now the promise is only created for the single warmup subscription, whose rejection is handled by the existing tap + catchError. No new unhandled rejection.
  • tsc is clean on src and on src + test, and the regenerated reference docs match a fresh npm run docs (just the Defined in line shifts from the added import).

One optional follow-up

Only useIdTokenResult has a regression test; useStorageDownloadURL and useCallableFunctionResponse got the identical fix but lean on that test as a proxy.

The mechanism is the same across all three, so this isn't a correctness concern, but a call-count test for the functions hook (where the eager re-call actually invokes a cloud function) would guard against a future regression.

You already offered these in the description, so this is just a +1. Non-blocking.

Approving.

Mirrors the useIdTokenResult defer test: asserts the underlying
callable is invoked once across re-renders. Addresses Armando's
non-blocking follow-up on FirebaseExtended#757.
getMockImplementation() erases the generic signature, breaking the
explicit type arguments on originalImpl<...>() under strict mode.
Cast to typeof httpsCallable to keep it. Failed Type check (React
18/19) in CI, tsconfig.json alone doesn't cover test/.
@tyler-reitz
tyler-reitz merged commit 38a6a4e into FirebaseExtended:main Jul 24, 2026
19 of 20 checks passed
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.

useCallableFunctionResponse invokes the cloud function on every re-render useIdTokenResult fires a redundant token fetch on every re-render

2 participants