Skip to content

Fix useIsAuthenticated hook briefly returning false even though the user is authenticated #7057

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

Merged
merged 9 commits into from
Apr 26, 2024

Conversation

hatched-kade
Copy link
Contributor

Addresses #6918

The current useIsAuthenticated has an unnecessary useEffect, which causes it to briefly return false in some cases even when an account is present. See https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state and https://react.dev/learn/you-might-not-need-an-effect#caching-expensive-calculations

This PR switches to use the useMemo recommendation

I added a unit test that passes now, but fails with the old implementation:

 FAIL  test/hooks/useIsAuthenticated.spec.tsx
  ● withMsal tests › useAuthenticated always returns true if user has an account

    expect(jest.fn()).toHaveBeenCalledTimes(expected)

    Expected number of calls: 0
    Received number of calls: 1

      78 |         expect(await screen.findByText("Has accounts")).toBeInTheDocument();
      79 |         expect(await screen.findByText("Is authed")).toBeInTheDocument();
    > 80 |         expect(invalidAuthStateCallback).toHaveBeenCalledTimes(0);
         |                                          ^
      81 |     });
      82 | });
      83 |

      at Object.<anonymous> (test/hooks/useIsAuthenticated.spec.tsx:80:42)

I also had to tweak some act / await / async code in other tests, presumably because now it takes fewer renders for the hook to start returning the correct value. Without the tweaks a couple tests were failing, and others printed warning about updating state outside of an act(...).

@github-actions github-actions bot added the msal-react Related to @azure/msal-react label Apr 25, 2024
@hatched-kade
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Giant Eagle"

Copy link
Collaborator

@tnorling tnorling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I imagine there's other places we can make similar changes.

Copy link
Collaborator

@jo-arroyo jo-arroyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, small nit

@tnorling tnorling merged commit c244947 into AzureAD:dev Apr 26, 2024
5 of 7 checks passed
@aleks-sch
Copy link

thanks @hatched-kade!

@tnorling @jo-arroyo could we (you) do a release of msal-react please?

we are also impacted by #6918 and would rather not install from dev branch. thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-react Related to @azure/msal-react
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants