Skip to content

Sync Logout across tabs #5190

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Sep 13, 2021
Merged

Sync Logout across tabs #5190

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9e4565f
logout fix
parasharrajat Sep 10, 2021
1d3369d
merge Session key updates
parasharrajat Sep 10, 2021
5c54266
remove issue link
parasharrajat Sep 10, 2021
fc4323c
comments
parasharrajat Sep 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions src/libs/actions/SignInRedirect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,21 @@ function redirectToSignIn(errorMessage) {
if (preferredLocale) {
Onyx.set(ONYXKEYS.NVP_PREFERRED_LOCALE, preferredLocale);
}
if (errorMessage) {
Onyx.set(ONYXKEYS.SESSION, {error: errorMessage});
}
if (activeClients && activeClients.length > 0) {
Onyx.set(ONYXKEYS.ACTIVE_CLIENTS, activeClients);
}

const session = {
// We must set the authToken to null so that signOut action is triggered across other clients
authToken: null,
};

if (errorMessage) {
session.error = errorMessage;
}

// `Onyx.clear` reinitialize the Onyx instance with initial values so use `Onyx.merge` instead of `Onyx.set`.
Onyx.merge(ONYXKEYS.SESSION, session);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not keep Onyx.set here, now that we combined error and authToken keys into one object?

Copy link
Member Author

@parasharrajat parasharrajat Sep 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

After, Onyx.clear, Onyx is initialized with the initial data https://github.com/Expensify/react-native-onyx/blob/6eadd7f73fca87e6551e43607ec78f645e17ef50/lib/Onyx.js#L618.

set here, was replacing that. So I used merge

App/src/setup/index.js

Line 30 in e8cdb02

[ONYXKEYS.SESSION]: {loading: false, shouldShowComposeInput: true},

Copy link
Contributor

Choose a reason for hiding this comment

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

Aah nice, thanks for pointing that out! 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

NAB, it might be worth adding a brief comment here explaining why we're not using a set(). That explanation makes sense to me, but someone might have the innocent idea to change this from a merge() to a set() and break the intended behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment added.

});
}

Expand Down