chore: forward-integrate main into v5#758
Conversation
…baseExtended#757) * fix: defer eager requests in storage, auth, and functions hooks 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. * docs: regenerate reference docs for defer import line shifts * test: add call-count regression test for useCallableFunctionResponse 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. * fix: preserve httpsCallable generics on mocked implementation 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/.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
armando-navarro
left a comment
armando-navarro
left a comment
There was a problem hiding this comment.
Thanks Tyler. I verified this is a clean forward-integration: the single commit here is the squash-merge of #757 (its parent is the current v5 tip), so merging fast-forwards v5 by exactly that one already-reviewed, already-shipped commit, with no conflicts and nothing extra.
I also re-checked the functions.test.tsx you added after my #757 approval (it addresses the functions-hook coverage I'd asked about): it passes, and reverting the defer makes it fail with four calls, so it's a real regression tripwire.
Two non-blocking notes:
mainhas moved two commits past this branch since it was cut, so this brings v5 up to #757 but not to main's current tip.- In particular it leaves out #760 (now tagged v4.2.6) and the version bump. Since v5 is still a strict ancestor of main, you could re-point this PR at main's current tip and fast-forward the whole way, folding #760 and the bump into one integration instead of needing a second sync right after.
- If stopping at #757 is deliberate, a note in the description would make that clear.
- The only failing check here is Publish (NPM); every functional check (build, both Node versions, both React type-checks) is green. Could you confirm that's the known publish flakiness rather than a new break?
Approving the integration.
|
Plan for this integration: going with Armando's suggestion to re-point at main's current tip rather than stopping at #757. Since v5 is still a strict ancestor of main, this stays a clean fast-forward (no conflicts). Re-pointing now folds in everything that has landed since this branch was cut: #757, #760, the 4.2.6 version bump, and the four Dependabot lockfile bumps (#761-764). Holding the re-point until Monday so we also capture anything that lands over the weekend and avoid a second sync right after. Re-pointing will move the head off the commit Armando approved, so it will need a fresh review (should be quick given it is a clean fast-forward). |
2 participants
Forward-integrates main into v5 (fast-forward, no conflicts) to bring in #757 (defer eager requests in storage/auth/functions hooks).
This lets #735's temporary storage
deferhunk dissolve on its next rebase, since that hunk was only kept to unblock the storage error test until #757 landed.No new changes beyond what's already on main: docs regen, src defer wraps in auth.tsx/functions.tsx/storage.tsx, and their tests. tsc (src + test configs) and the functions/auth emulator suites pass locally on Node 22.