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

Fix react-sdk issue where isLoggedIn being set to false after successful token refresh #169

Merged

Conversation

JakeLo123
Copy link
Collaborator

What is this PR and why do we need it?

Fixing issue #166.

This should ensure that the main provider for the react-sdk doesn't flip isLoggedIn when a token refresh is successful by placing the code to getNewCore inside the provider to keep it in the react life cycle. Using a mutable ref object.

@synedra -- tagging you as a reviewer in case you'd like to take a look and make sure I didn't mistake your nice work on the dispose method. If I've misconstrued something, please let me know.

Pre-Merge Checklist (if applicable)

  • Unit and Feature tests have been added/updated for logic changes, or there is a justifiable reason for not doing so.

@JakeLo123 JakeLo123 added bug Something isn't working React labels Sep 19, 2024
@JakeLo123 JakeLo123 self-assigned this Sep 19, 2024
Copy link
Contributor

@synedra synedra left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thank you so much for doing this!

synedra
synedra previously approved these changes Sep 19, 2024
Copy link
Contributor

@synedra synedra left a comment

Choose a reason for hiding this comment

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

Forgot to approve

@JakeLo123 JakeLo123 force-pushed the 166-fix-isLoggedIn-set-to-false-after-token-refresh branch from f0e1698 to aa4827b Compare September 19, 2024 19:58
@dmackinn
Copy link
Collaborator

@synedra can you re-approve when you have a moment, Jake pushed up an update to address linting concerns after the original approval.

synedra
synedra previously approved these changes Sep 19, 2024
Copy link
Contributor

@synedra synedra left a comment

Choose a reason for hiding this comment

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

Sorry I missed that one! Thanks for fixing...

@MaxDNG
Copy link

MaxDNG commented Sep 20, 2024

Well done! If I may add, I think the config object should still be destructured. As it stand, because the rest operator is used, we retrieve a new config object on each re-render, making the useMemo not really useful.
The two options I see is either to extract all keys of the config from the FusionAuthProvider props, or to move it to a single config props. I think the second option is less desirable as it introduces a breaking change and puts the responsibility of maintaining the object reference (memoization) to the user.

@JakeLo123
Copy link
Collaborator Author

@MaxDNG That's a good suggestion -- I've pushed a commit to use destructuring instread of the rest operator to get the config.

I was able to tell that this regression happened on commit 6bbb2a7 -- a commit that I made 😅 -- where we switched the definition of FusionAuthProvider to use destructuring. I'm honestly not sure how exactly that causes this behavior but it is indeed the cause.

If anyone would like to observe it for yourself...

  • Check out to commit 657c18d and observe that the isLoggedIn behavior is correct.
  • Then checkout 6bbb2a and observe the behavior is buggy.
  • Switch back to 657c18d and destructure the config object, either in a useMemo body or on the lines where you pass the properties into new SDKConfig
  • Observe that the behavior is correct.

@JakeLo123
Copy link
Collaborator Author

@synedra -- sorry I keep dismissing your PR approval.

Copy link
Contributor

@alex-fusionauth alex-fusionauth left a comment

Choose a reason for hiding this comment

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

Tested locally, thanks @JakeLo123 and @MaxDNG for jumping in!!

@alex-fusionauth alex-fusionauth merged commit d80ae45 into main Sep 20, 2024
3 checks passed
@alex-fusionauth alex-fusionauth deleted the 166-fix-isLoggedIn-set-to-false-after-token-refresh branch September 20, 2024 14:12
This was referenced Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working React
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants