-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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(chore): replace use-context-selector context with React Context #20287
fix(chore): replace use-context-selector context with React Context #20287
Conversation
From your PR description, it's not clear how you came to this change, can you explain it in detail? It's a very "big" change to make across the app because we loose selection performance, so good to understand it thoroughly :) |
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.
Great investigation. This would have been very tricky to debug, to do this properly we have a few options:
- remove
use-context-selector
as you have done, but also remove the selector function since it's pointless and verbose now. - use a unique version with the
Auth
feature i.e.@radix-ui/react-context
that doesn't have the selector used and would also solve the issue, but we do risk this quirk coming back for us in the future, although I don't think it will considering how unique this is to the router acting before react has updated. - We could potentially pass the
redirect
query param a bit more and inlogin
check if it's there and just do it if we dont actually need the user to login,return <Redirect to="/" />;
Let me know what you think!
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hey @joshuaellis , thanks a lot for your suggestions... In the end I decided to continue with solution 1 because it lets me use most of your code and just change some small parts in the code (remove the selectors). |
…after-registration
Size Change: -1.21 kB (-0.09%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
we'll need to check there aren't performance regressions, but i'm sure it'll be fine :) |
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.
code changes look fine ✅
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.
Did QA, merging now as Simone is OOO
What does it do?
Fix the issue we had after registration because the token was not updated correctly when we are redirected to the usecase page
Why is it needed?
With use-context-selector context when we save the token in the registration handler, using the setToken coming from the useAuth hook, the value is not immediately updated to be used in the PrivateRoute component so the UseCasePage page is not served
How to test it?
Related issue(s)/PR(s)
Related issue: #20345