Skip to content
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

Initialized flag on useAuth0 hook #561

Merged
merged 7 commits into from
Jan 26, 2023

Conversation

cranberyxl
Copy link
Contributor

@cranberyxl cranberyxl commented Nov 22, 2022

Changes

Add isInitializing flag to useAuth0. This is true until we know that a user is either logged in or logged out. This allows us to prevent showing the logged in or logged out state of an app incorrectly while the user is returned from the CredentialsManager.

Testing

const MyComponent = () => {
  const { isInitializing, user } = useAuth0();

  if (isInitializing) {
    return <Text>Auth0 is initializing</Text>;
  }

  return <Text>User is logged { user ? 'in' : 'out' }</Text>;
}
  • This change adds unit test coverage
  • This change has been tested on the latest version of the platform/language or why not

Checklist

@cranberyxl cranberyxl requested a review from a team as a code owner November 22, 2022 10:27
@poovamraj
Copy link
Contributor

@cranberyxl I guess this can be achieved by checking both user object and error object by checking if one in both of them are initialized. Is there any other particular scenario I am missing out?

@cranberyxl
Copy link
Contributor Author

There is the time before the first request has completed, where there is no user and no error, while the first calls are being made to credentials manager, etc. This is indistinguishable from there is no user and no error and we've checked credentials manager.

@DannyBiezen
Copy link

This is something I've been wanting as well. It would help cleanup some of the current authentication logic in my components

Copy link
Contributor

@poovamraj poovamraj left a comment

Choose a reason for hiding this comment

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

Hi @cranberyxl, this would be a great addition indeed. Can you use our auth0-react as inspiration and make the following changes?

  • Rename the state to isLoading
  • Set it to false when the user is loaded from Credentials Manager or Login
  • Set it to false when there is an error
  • Set it to true when the Login is started again.

@gradisarjoze
Copy link

Anyone with any workarounds at the moment?

@poovamraj
Copy link
Contributor

Hi everyone, thanks a lot for the contribution. We have made a few more refactors and improvements to this PR. Please feel free to let us know your feedback on this. Thanks!

Copy link
Contributor Author

@cranberyxl cranberyxl left a comment

Choose a reason for hiding this comment

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

Looks good to me

@cjpete
Copy link

cjpete commented Jan 25, 2023

Looks great. It would be great to see a similar PR to https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/react-native-auth0 to keep the two in sync

@poovamraj
Copy link
Contributor

@cjpete Great catch. Any possibility you can help us with it?

@cranberyxl
Copy link
Contributor Author

@poovamraj I think isInitializing is a more semantically correct name as the flag is only changed once. If new requests are made it does not flip back to true

@frederikprijck
Copy link
Member

frederikprijck commented Jan 26, 2023

@cranberyxl It was decided to go with isLoading for consistency because this is what is already being use in auth0-angular, auth0-react and auth0-vue to represent the, almost, identical behavior and we believe consistency is not unimportant, especially to improve DX for people working with both auth0-react and react-native-auth0. Even though auth0-react and react-native-auth0 are two different SDKs, we believe consistency between those two will benefit you as a user.

@poovamraj poovamraj merged commit 5de06c1 into auth0:master Jan 26, 2023
@poovamraj poovamraj mentioned this pull request Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants