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 18 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( | ||
| 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; | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
@truph01 This feels like a place where we can use
Onyx.connectWithoutViewvariable honestly, which can be later replaced by a simpleOnyx.get. Otherwise, the call spans way too much and makes some components harder to make React compiler compliant later.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.
cc @mountiny @fabioh8010 for opinions.
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.
What do you think about this comment @mountiny @tgolen @fabioh8010 ?
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 think we are a long way away from having a more simple
Onyx.get()so unless there is a performance concern, I would like to avoid usingconnectWithoutView()here.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.
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.
@tgolen We are suppressing warnings in a lot of places with
eslint-disable-next-line react-hooks/exhaustive-depsas we can't add session to the deps for this function as it will trigger unexpected logouts (which is a natural potential churn event). But doing this makes the component non-optimizable by the React compiler.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.
We have other functions need to be migrated like buildOnyxDataToCleanUpAnonymousUser, waitForUserSignIn, ...
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 noticed that we use the ref approach in a few places, where the ref is accessed inside
useEffectto avoid re-running the effect when the value is updated.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.
@truph01 Please keep this PR on hold and continue with other functions that are easy to migrate.
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.
We are moving forward with introducing
Onyx.get()so that will help to unblock this. You can follow along with #101910 to watch it roll outThere 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.
Thanks @tgolen! Updated to use
Onyx.get():SESSIONto pass it tosignOutAndRedirectToSignIn(AnonymousReportFooter,SignInButton,GenericErrorPage,useSignOut) now read it lazily withOnyx.get(ONYXKEYS.SESSION)in the press handler, so there is no new subscription, no re-render, and noexhaustive-depssuppression.Link.tsdeep-link sign-out andHandleDeletedAccountmiddleware) can't useOnyx.getyet because ofno-unsafe-onyx-read. They use aconnectWithoutViewmirror for now;Link.tsreuses its existing session subscription.cc @shubham1206agra