-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix useAuthProvider
may return undefined
when no authProvider
is available
#9861
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple nitpicks, but apart from that, 👍
}, | ||
[resetStore, location, navigate, loginUrl, queryClient] | ||
[authProvider, resetStore, loginUrl, queryClient, location, navigate] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is problematic: location
changes on every navigation, so the callback memoization is weak. You're using a locationRef for the sarch and hash, why not for the pathName?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why it was not done for pathName
(I just moved the content of the second useCallback
). But I assume we probably simply forgot to change this second callback to use refs while we changed the first callback, 2 years ago. Besides I can't think of any negative consequences this might have, so I used refs in this second part, too.
Nice catch!
timer = setTimeout(() => { | ||
timer = undefined; | ||
}, 0); | ||
? authProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this method is a bit too long for a legible ternary. I'd prefer a leave early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
Fixes BC introduced by #9760 (comment)