-
Notifications
You must be signed in to change notification settings - Fork 266
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
[SDK-2106] Memoize auth methods #150
Conversation
b10637f
to
c560483
Compare
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 love this, I added a comment but I don't think it is blocking.
const memoized = result.current.getIdTokenClaims; | ||
rerender(); | ||
expect(result.current.getIdTokenClaims).toBe(memoized); | ||
}); |
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.
Even tho this is mostly just testing that we are using useCallback and that it is working as expected, I actually like this test.
Would it make sense to add a test for the other functions as well?
Could probably make it a single test:
[
{ name: 'getIdTokenClaims', cb: (provider) => provider.getIdTokenClaims },
{ name: 'loginWithRedirect', cb: (provider) => provider.loginWithRedirect },
].forEach(({name, cb}) =>
it(`should memoize the ${name} method`, async () => {
const wrapper = createWrapper();
const { waitForNextUpdate, result, rerender } = renderHook(
() => useContext(Auth0Context),
{ wrapper }
);
await waitForNextUpdate();
const memoized = cb(result.current);
rerender();
expect(cb(result.current)).toBe(memoized);
})
);
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.
Good idea - I'd do this but my IDE doesn't like dynamically creating tests - it doesn't let me run them from the gui. (I know that shouldn't stop me from doing it, but I really like running my tests like that 🤷)
(will base onmaster
after #146 is merged)Description
Memoizing the auth methods means that downstream components that use them in dependent keys of other hooks wont get unnecessary updates when auth state changes.
*Easier to review if you hide whitespace changes https://github.com/auth0/auth0-react/pull/150/files?w=1
References
Fixes #131
Testing
Checklist
master