-
-
Notifications
You must be signed in to change notification settings - Fork 153
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] Call SIGNED_OUT
event where session is removed
#854
base: master
Are you sure you want to change the base?
Conversation
Any feedback on this @kangmingtay @hf ? We're needing to patch this library whenever a new version is released. |
Any updates here? |
src/GoTrueClient.ts
Outdated
if (currentSession) { | ||
await this._saveSession(currentSession) | ||
} |
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'm not sure why it's necessary to preserve the existing session when one calls the signup method - we always want to remove the session when signup is called to prevent any possibilities of the user logging in as someone else.
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.
this applies to all the other sign-in methods too - if you're signing in, there won't be any session in the first place, else you'd be logged in already
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.
This truly depends on the UI implementation. We may need to allow logged-in users to switch accounts directly so it is possible to call sign-in/sign-up. Currently, if something fails, the user is kicked out of their current session.
In addition, I don't see how this would log a user into someone else's account. Either the sign-in is successful, and the current session switches to the new one, or it fails, and you keep the existing one. If you were never logged in, you stay logged out.
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.
If the team wanted to go down this path, you could simplify a lot of these methods by moving the _removeSession call to right before the _saveSession calls. There would be a few exceptions to that though.
I've seen more and more discussions about people trying to implement a "switch account" feature; or they already have, but recent changes have broken their implementations.
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 agree. I just wasn't sure if some logic was dependent on having no session while executing, so I preferred keeping it and reverting at the end.
Hopefully, we can remove the current limitation, be it by removing _removeSession
where not necessary or, as my PR suggests, restoring the session on fail.
This is already working for us in production, we patch this library in our app for this behavior, but we'd prefer if the lib had it working natively.
Any updates on this, it's breaking our production Electron app, seems that when a request fails, the user just gets signed out entirely. |
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 appreciate the review tag, but this one will ultimately need to be looked at by kangmingtay or hf. Last I knew, the team's time was limited; so, who knows if/when they'll consider this PR further.
Ok, I'll mention @hf so they'll be aware and have visibility on this fix request. |
sorry @bombillazo i was on vacation last week - i'll be looking into this further to see if we can just do away with removing the session prematurely before hitting the underlying auth endpoint - not too sure if it will cause any unintended breaking changes here so we need some time to test this out |
@bombillazo i've made a separate PR (#915) to address this - would be great if you can take a look at it too |
## What kind of change does this PR introduce? * Replaces #854 * Fixes #853, #904 * We don't need to remove the existing session prematurely. This causes some issues when users want to implement some sort of switch-account functionality since the existing session will always be removed regardless of whether the signup / sign-in attempt succeeds. * It's safe to remove `_removeSession` since calling `_saveSession` multiple times will just replace the existing session
Hey @kangmingtay thanks for the PR, I merged the current repo to my branch and the only thing missing is a few lines of code to emit the |
SIGNED_OUT
event where sessino is removed
SIGNED_OUT
event where sessino is removedSIGNED_OUT
event where session is removed
Any update here? This fixes #853 |
Hey @kangmingtay , any update on this issue? |
What kind of change does this PR introduce?
This adds the SIGNED_OUT event missing in some logic that clears/logs out the session.
What is the current behavior?
#853