Repository navigation
[Part 2] Remove Onyx.connect(ONYXKEYS.SESSION) in /Session/index.ts: thread sesion through signOutAndRedirectToSignIn #97963
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
5de8203
73939bc
014e759
ecf1362
0d2b466
abafbd4
6d3bcda
1c4427a
93e84dd
12678ba
5bbb459
7611b41
0cdbc71
fbf04d8
1f059d5
1bfe1f5
5928078
4004609
6c5ca28
8b4f74b
cd396ca
66833a6
4c86812
22bf041
fa58304
a003e62
0603298
5a145ee
4947c9b
a5ee201
7463da2
7c404d6
119adb9
779004e
1d15600
750727d
3903963
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -353,7 +353,13 @@ const KEYS_TO_PRESERVE_SUPPORTAL = [ | |
| ONYXKEYS.COLLECTION.DEVICE_BIOMETRICS, | ||
| ]; | ||
|
|
||
| function signOutAndRedirectToSignIn(shouldResetToHome?: boolean, shouldStashSession?: boolean, shouldSignOutFromOldDot = true, shouldForceUseStashedSession?: boolean) { | ||
| function signOutAndRedirectToSignIn( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @truph01 This feels like a place where we can use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @mountiny @fabioh8010 for opinions.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about this comment @mountiny @tgolen @fabioh8010 ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we are a long way away from having a more simple
I don't have a whole lot of context behind this though, so if it is a real problem, I'd like to understand more about this and what problem it creates with the React compiler.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@tgolen We are suppressing warnings in a lot of places with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have other functions need to be migrated like buildOnyxDataToCleanUpAnonymousUser, waitForUserSignIn, ...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I noticed that we use the ref approach in a few places, where the ref is accessed inside
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @truph01 Please keep this PR on hold and continue with other functions that are easy to migrate.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are moving forward with introducing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @tgolen! Updated to use
|
||
| shouldResetToHome?: boolean, | ||
| shouldStashSession?: boolean, | ||
| shouldSignOutFromOldDot = true, | ||
| shouldForceUseStashedSession?: boolean, | ||
| session: OnyxEntry<Session> = deprecatedSession, | ||
| ) { | ||
| Log.info('Redirecting to Sign In because signOut() was called'); | ||
| hideContextMenu(false); | ||
|
|
||
|
|
@@ -367,7 +373,7 @@ function signOutAndRedirectToSignIn(shouldResetToHome?: boolean, shouldStashSess | |
| return; | ||
| } | ||
|
|
||
| const isSupportal = isSupportAuthToken(); | ||
| const isSupportal = isSupportAuthToken(session); | ||
| const shouldRestoreStashedSession = isSupportal || shouldForceUseStashedSession; | ||
|
|
||
| if (CONFIG.IS_HYBRID_APP && shouldSignOutFromOldDot) { | ||
|
|
@@ -379,7 +385,7 @@ function signOutAndRedirectToSignIn(shouldResetToHome?: boolean, shouldStashSess | |
| // session. | ||
| const signOutPromise: Promise<void | Response<never>> = | ||
| !shouldRestoreStashedSession && !shouldStashSession | ||
| ? signOut({autoGeneratedLogin: credentials?.autoGeneratedLogin, signedInWithSAML: !!deprecatedSession.signedInWithSAML, authToken: deprecatedSession.authToken}) | ||
| ? signOut({autoGeneratedLogin: credentials?.autoGeneratedLogin, signedInWithSAML: !!session.signedInWithSAML, authToken: deprecatedSession.authToken}) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When callers now pass an explicit session to avoid relying on module-level Onyx state, this branch still sends Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the next PR, we will remove |
||
| : Promise.resolve(); | ||
|
|
||
| // The function redirectToSignIn will clear the whole storage, so let's create our onyx params | ||
|
|
@@ -392,7 +398,7 @@ function signOutAndRedirectToSignIn(shouldResetToHome?: boolean, shouldStashSess | |
| if (!isSupportal && shouldStashSession) { | ||
| onyxSetParams = { | ||
| [ONYXKEYS.STASHED_CREDENTIALS]: credentials, | ||
| [ONYXKEYS.STASHED_SESSION]: deprecatedSession, | ||
| [ONYXKEYS.STASHED_SESSION]: session, | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -414,7 +420,7 @@ function signOutAndRedirectToSignIn(shouldResetToHome?: boolean, shouldStashSess | |
| newDotCurrentAccountEmail: stashedSession.email ?? '', | ||
| authToken: stashedSession.authToken ?? '', | ||
| policyID: '', | ||
| accountID: deprecatedSession.accountID ? String(deprecatedSession.accountID) : '', | ||
| accountID: session.accountID ? String(session.accountID) : '', | ||
| }); | ||
| hasSwitchedAccountInHybridMode = true; | ||
| } | ||
|
|
||
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.
When the web HR flow stays mounted after
openLinkopens the OldDot setup URL, any subsequentSESSIONOnyx update re-runs this effect becausesessionis now in the dependency array, opening another setup URL/tab for the same click. The native variants guard this as a one-time open; keep the web flow mount-only as well, while still passing the latest session toopenLink.Useful? React with 👍 / 👎.
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.
Resolved via 6d3bcda