feat(auth)!: remove the deprecated AuthCheck and ClaimsCheck components - #782
feat(auth)!: remove the deprecated AuthCheck and ClaimsCheck components#782tyler-reitz wants to merge 3 commits into
Conversation
armando-navarro
left a comment
There was a problem hiding this comment.
Thanks for splitting this out, and for writing up the vacuous test rather than quietly deleting it. Checks have gone green since the outage cleared, including the reference-docs check, so the gap you flagged at the top of the description is closed.
That test finding holds, incidentally. I put an assertion that cannot pass inside UserDetails on v5 and the suite still went 16 green, so the component really never mounted. Your replacement is real too: pull the signIn() and it fails.
The removal itself I could not find a problem with. I swept for surviving references across src/, both example/ variants, the README, the docs and the packaging files, and there are none. Everything below is the docs and one test.
The upgrade guide's non-suspense sentence is wrong in both halves
This is the one I care about, because the guide is what someone migrating to v5 actually reads.
docs/upgrade-guide.md:9 says they "only ever worked with <FirebaseAppProvider suspense={true}>, and in non-suspense mode they logged a deprecation warning and rendered anyway." I built both shapes against v5 and ran them. Each half is wrong for a different component.
- Plain
AuthCheckworked fine without suspense. Fallback when signed out, children after sign-in, and straight to the children with no fallback pass when already signed in, sinceuseUserseedsinitialDatafromauth.currentUser. The cost was the warning. So "only ever worked with suspense" is not true for it. AuthCheckwithrequiredClaimswarned and then threwError: ClaimsCheck must be run in Suspense mode, so "rendered anyway" is not true for it. The warn sits above theif (user)branch so it fires either way, andClaimsCheckonly renders onceuseris truthy, so the throw arrives when a signed-in user reaches the claims check.ClaimsCheckon its own could throw before it ever warned. Itsif (status === 'loading') throwsits above bothuseSuspenseEnabledFromConfigAndContext()and its ownconsole.warn, anduseIdTokenResultdefers an async call, so a cold entry isloadingon first render. That also makes "you have been seeing that warning" wrong for those users.
The person most exposed is the one running plain AuthCheck without suspense. Their app works today and the guide tells them it never did, so they skip the entry, and for a plain JS importer the removal then breaks at load rather than at compile. Replacing both sentences of :9:
ClaimsCheck, andAuthCheckwithrequiredClaims, only ever worked with<FirebaseAppProvider suspense={true}>. In non-suspense mode they threwError: ClaimsCheck must be run in Suspense modeas soon as a signed-in user reached the claims check.AuthCheckwithoutrequiredClaimsdid work in non-suspense mode. BothAuthCheckshapes logged a deprecation warning first, butClaimsCheckon its own threw before reaching its warning, so you may have seen no warning at all.
The same claim is in 277f293's body, which this repo pre-fills as the squash commit body (squash_merge_commit_message: COMMIT_MESSAGES). Whoever merges can edit it in the box, but only if they notice, and no later commit can. Two other things in there while you are at it: "four years and three months" is over five (5 years 2 months 23 days from #368 merging 2021-05-14 to 277f293 on 2026-08-06), and "Nothing in the repo has pointed at them since" is contradicted further down by "the four AuthCheck tests go."
One test stopped covering useUser
does not show a logged-out user after navigating away sits in describe('useUser') and nothing in its tree calls useUser any more. AuthCheck opened with useUser<User>(). SigninGate calls useSigninCheck(). I made useUser throw and ran that test on both branches: v5 fails, this branch passes. So "still tests what it tested" is not accurate, and neither is the comment at test/auth.test.tsx:31-33.
It is a trade rather than pure loss, to be fair. SigninGate is now the only place useSigninCheck runs in a real tree with suspense enabled, since the five renderHook tests all use the non-suspense Provider. It never actually suspends there, because test/auth.test.tsx:100 populates that same cache entry first and nothing deletes it, which is also why deleting the <React.Suspense> boundary changes nothing. That part is not yours: deleting it on v5 passes too.
#754 called this shot: its Group 3 notes say that test "needs rewriting to use a plain provider, not removing."
const SigninGate = ({ children }: { children?: any }) => {
const { data: user } = useUser();
if (!user) {
return <h1 data-testid="signed-out">not signed in</h1>;
}
return <>{children ?? <h1 data-testid="signed-in">signed in</h1>}</>;
};That is what AuthCheck did before delegating to ClaimsCheck, and these tests never passed requiredClaims. It passes 12/12, typechecks, fails again under the useUser mutation, and makes the :31-33 comment true again. If you would rather keep useSigninCheck, then that comment needs updating too, and moving the test out of describe('useUser') and renaming it would at least stop it claiming coverage it does not have.
Smaller: the migration sample throws if the hook errors
docs/upgrade-guide.md:20-24 guards loading and then reads signInCheckResult.signedIn. I ran that shape against an observable erroring before its first emission, in non-suspense mode: status comes back error, data comes back undefined, and it throws TypeError: Cannot read properties of undefined (reading 'signedIn'). TypeScript will not catch it, since ObservableStatus<T> declares data: T rather than a discriminated union. Two changes, since error is not destructured today:
const { status, data: signInCheckResult, error } = useSigninCheck({ requiredClaims: { admin: true } });
if (status === 'loading') return <LoadingSpinner />;
if (status === 'error') return <ErrorPage error={error} />;Reachable in practice, since the requiredClaims path calls getIdTokenResult() (src/auth.tsx:149). Worth doing here rather than later because the next section of that same file is the change that makes status: 'error' reachable at all, and states at :45 that checking status first is enough. The guide's own sample checks status only for loading, so a reader comparing the two answers "yes I do" and moves on. The same shape is in docs/use.md and the useSigninCheck JSDoc, which I would leave for a separate pass.
Answering what you raised
useIdTokenResult and the .tsx extension: agreed on both, keep them as they are. "Not in this PR": your read matches mine, Group 1 blocked on #740, Group 2 wanting its own PR, Refs rather than Fixes. Your "Noted, not fixed" coupling reproduces too, including the second failure you called out, returns the same value as getAuth(app).currentUser. And please do file the inert-check pattern. Three of the same shape is the more interesting thing here, and a test that asserts nothing is worse than a missing one because it reads as coverage.
Two counts are off and change nothing: the reference-doc modified count is 10, and the eslint row is a two-file run rather than npm run lint, which still carries the pre-existing require error in test/setupTests.ts.
To approve
Two things: docs/upgrade-guide.md:9 plus the matching sentence in the commit body, and the useUser coverage restored either way above. The rest is optional and I would take it in a follow-up.
Nothing in the shipped code is wrong. The guide is what makes this blocking for me, because it is the one document that exists to be read during a breaking migration and it currently describes the wrong behavior.
If I have misread any of this, particularly the non-suspense behavior, say so and I will run it again.
Both were deprecated in FirebaseExtended#368 (2021-05-14), which shipped useSigninCheck as their replacement in the same commit and migrated docs/use.md and the example app off them. That is five years and two months of deprecation. Nothing in src/, the docs or the example has pointed at them since; only tests did. What they actually did without suspense, verified by rendering each shape against v5 rather than read off the source: - Plain AuthCheck worked. Fallback when signed out, children after sign-in, and straight to the children when already signed in, since useUser seeds initialData from auth.currentUser. The cost was the deprecation warning. - AuthCheck with requiredClaims warned, then threw "ClaimsCheck must be run in Suspense mode" once a signed-in user reached the claims check. - ClaimsCheck on its own could throw before it ever warned: its loading throw sits above both the suspense-mode lookup and its own console.warn. So the user most exposed by this removal is the one running plain AuthCheck without suspense, whose app works today. The upgrade guide says so. Removing them is a runtime break for a plain JS importer, not just a type error, so it rides the major. Also removes the exported AuthCheckProps and ClaimsCheckProps. ClaimCheckErrors stays: it sits between them in the file but belongs to the SigninCheckResult shape that useSigninCheck returns. src/auth.tsx drops its `React` and `useSuspenseEnabledFromConfigAndContext` imports, which are used only by the removed code and would fail the build under noUnusedLocals. The file keeps its .tsx extension despite no longer containing JSX; renaming emits the same auth.js and only costs blame history. Tests: the four AuthCheck tests go. Two others used the AuthCheck wrapper without being about AuthCheck, so they get a useUser-based gate that renders the same testids. useUser is what AuthCheck itself called before delegating to ClaimsCheck, and these tests never passed requiredClaims, so the tests in describe('useUser') keep covering useUser. Mutation-verified: making useUser throw fails them. One of those was vacuous and is now real. `beforeEach` signs out, so the old gate rendered its fallback, UserDetails never mounted, and its two expectations never executed. It now signs in first and awaits the gated testid. Mutation-verified: removing the sign-in fails it. Group 3 of FirebaseExtended#754. Groups 1 and 2 are unaffected, so FirebaseExtended#754 stays open.
`console.info` is mocked in `beforeAll` to silence the Auth Emulator warning. The restore lived in an `afterAll` nested inside another `afterAll`, so the inner hook was only registered while teardown was already running, and never ran. The mock leaked past the suite. Pre-existing and unrelated to the AuthCheck removal, folded in because it is two lines in a file this branch already edits. The `@ts-expect-error` above it is still required, which the test typecheck confirms.
…seUser coverage
Both from Armando's review.
The guide said the components "only ever worked with suspense={true}, and in
non-suspense mode they logged a deprecation warning and rendered anyway".
Wrong in both halves, and wrong in opposite directions. Rendered each shape
against v5:
- Plain AuthCheck worked in non-suspense mode, warning aside.
- AuthCheck with requiredClaims warned, then threw.
- ClaimsCheck alone threw before reaching its warning, so those users may
have seen no warning at all.
The person most exposed is the one running plain AuthCheck without suspense.
Their app works today, the old text told them it never did, so they would
skip the entry and hit a load-time break.
The SigninGate stand-in now uses useUser rather than useSigninCheck, so the
tests inside describe('useUser') cover useUser again. Verified both ways:
with the gate on useSigninCheck, making useUser throw left the navigating-away
test passing while the same mutation failed it on v5. With the gate on
useUser it fails again. Suite is 12/12, both typechecks clean.
Also from the review: the migration sample guarded loading and then read
signInCheckResult.signedIn, which throws a TypeError when the observable
errors before its first emission, since ObservableStatus declares data: T
rather than a discriminated union. It now destructures error and checks
status === 'error'.
652440a to
11aac86
Compare
|
Both fixed, and you read the non-suspense behavior correctly. I rendered all three shapes against One thing worth knowing if you re-run it: the
I also took the migration sample fix, since it was two lines in the same file. Heads up on history: I rewrote the commit body, so Filing the inert-check issue next. |
armando-navarro
left a comment
There was a problem hiding this comment.
Both of these are done, and the coverage one holds up when I re-run it:
- Making
useUserthrow now fails all five tests indescribe('useUser'). - On the previous head that same mutation left the navigating-away test passing.
That clears what I blocked on. One correction below.
One correction, to my own wording
You said the ClaimsCheck case only reproduces on a cold cache. That is right, and I had only ever run it cold before handing you that paragraph. Re-ran it against v5 in non-suspense mode, one uid per case so the auth:idTokenResult:<uid>:forceRefresh=false entry is never shared:
- Cold entry, signed in:
ClaimsCheckalone andAuthCheckwithrequiredClaimsboth throw. - Signed out,
AuthCheckwithrequiredClaims: no throw at all, it returns the fallback without ever reaching the claims check. - Warm entry, token already fetched: neither throws. Children or fallback depending on the claims, and the
AuthCheckshape logs two warnings, its own and thenClaimsCheck's. - Remount after its own first throw, same user: renders. The render that threw still constructs the cached observable, whose constructor subscribes immediately, so the retry finds the token loaded.
So "only ever worked with <FirebaseAppProvider suspense={true}>" is stronger than what actually happened. The condition was whether that user's token result was already loaded, not whether suspense was on.
I am not sending you another rewrite of that paragraph, for two reasons:
- The shipped text already gets the case that matters most right, in saying plain
AuthCheckworked without suspense. - Anyone left in the remaining case has to migrate either way.
If you do want it tightened, that loaded-or-not condition is what to tighten it to.
Small one: 5430ef2's body describes code that arrives in 11aac86
It says those two tests "get a useUser-based gate" and closes "Mutation-verified: making useUser throw fails them".
- The gate is still
useSigninCheck()at5430ef2and atfc8d838. - It becomes
useUser()only in11aac86. - A squash defuses it, since the merged tree really does have the
useUsergate. The concatenated body just reads oddly, saying both that those tests "get a useUser-based gate" and, later, that the gate "now uses useUser rather than useSigninCheck".
It only really bites if this ever lands as separate commits, where the first would carry a verification that is false for its own contents.
Approving. Nothing above blocks, so whether to touch the guide wording is your call. If I have any of these backwards, tell me which one and I will run it again.
Group 3 of #754. Refs #754, which stays open for Groups 1 and 2.
What this removes
AuthCheckandClaimsCheck, plus their exportedAuthCheckPropsandClaimsCheckPropstypes.They were deprecated in #368 on 2021-05-14, which shipped
useSigninCheckas the replacement in the same commit and migrateddocs/use.mdandexample/withoutSuspense/Auth.tsxoff them. Five years and two months. Nothing insrc/, the docs or the example has pointed at them since; only tests did.What they actually did without suspense, corrected after Armando's review and verified by rendering each shape against
v5rather than reading the source:AuthCheckworked. Fallback when signed out, children after sign-in, and straight to the children when already signed in, sinceuseUserseedsinitialDatafromauth.currentUser. The cost was the deprecation warning.AuthCheckwithrequiredClaimswarned, then threwClaimsCheck must be run in Suspense modeonce a signed-in user reached the claims check.ClaimsCheckon its own could throw before it ever warned, because its loading throw sits above both the suspense-mode lookup and its ownconsole.warn.So the user most exposed by this removal is the one running plain
AuthCheckwithout suspense, whose app works today. The upgrade guide says exactly that.Removing them is a runtime break for a plain JS importer (an import error at load), not just a type error, which is why this rides the major rather than a minor.
Deliberately kept
ClaimCheckErrorsstays. It sits directly between the two removed interfaces insrc/auth.tsxand is easy to take by accident, but it belongs to theSigninCheckResultshape thatuseSigninCheckreturns.useIdTokenResultstays too: its only in-repo caller wasClaimsCheck, but it is public API in its own right.Two consequences worth reading
1.
src/auth.tsxloses two imports, and a naive removal would not build.tsconfig.jsonsetsnoUnusedLocals, and everyReact.reference in the file plus every use ofuseSuspenseEnabledFromConfigAndContextlived inside the removed code. Both imports go. The file now contains no JSX; the.tsxextension stays, since renaming emits the sameauth.jsand only costs blame history.2. One of the tests I had to touch was vacuous, and it is now real.
it('always returns a user if inside an <AuthCheck> component')was never aboutAuthCheck.beforeEachsigns out, so the gate rendered its fallback,UserDetailsnever mounted, and both of itsexpectcalls never executed. It now signs in first and awaits the gated testid. Mutation-verified: removing thesignIn()makes it fail. Filed with the other instances as #788.The reference docs show 9 more modified files than the deletions alone, all source-link line numbers moving because
src/auth.tsxlost 89 lines. No content changed in any of them.Test changes
describe('AuthCheck')block goes: 4 tests plus atest.todo, all of which existed only to exercise the removed component.AuthCheckWrapperwithout being aboutAuthCheck. They now share aSigninGateWrapperbuilt onuseUser, which is whatAuthCheckitself called before delegating toClaimsCheck, so the tests indescribe('useUser')keep coveringuseUser. Mutation-verified in both directions: an earlier revision built the gate onuseSigninCheck, and under a throwinguseUserthe navigating-away test passed here while the same mutation failed it onv5. With the gate onuseUserit fails again.Noted, not fixed
The auth tests are coupled through the global observable cache. Under the mutation above, a second unrelated test also failed (
returns the same value as getAuth(app).currentUser), because theauth:user:cache entry persists across tests. Green normally, pre-existing, but it is the sameglobalThiscache that per-request SSR scoping has to address. It is also why theClaimsCheck-throws-before-warning behaviour above only reproduces on a cold cache.One unrelated fix, folded in deliberately (second commit)
test/auth.test.tsxhad anafterAllnested inside anafterAll, so the innerconsole.info.mockRestore()was registered only once teardown was already running and never ran. The mock leaked past the suite. Two lines, separate commit so it can be read or reverted on its own.Not in this PR
Groups 1 and 2 of #754. Group 1 (
checkOptions,checkinitialData) is blocked:checkIdFieldstill callscheckOptionsonv5, and the rewrite that removes it lives on #740's branch, which targetsmainand has not merged. Group 2 (startWithValue) is a behavior change with three live call sites and wants its own PR. #754 cannot close until Group 1 unblocks.