Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
Hey why is this necessary
redirectToSignIn
is clearing the whole storage anyway?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.
For some reason
redirectToSignIn
was failing to clear the storage and discard theauthToken
. This PR solved an issue where after being redirected, the user could refresh the page and would be logged in again.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 was the reason?
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.
Not sure yet. I'll investigate it as part of https://github.com/Expensify/Expensify/issues/194430
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.
Update - it seems that
Onyx.clear
is slow. In my tests, it took about 2.3s forOnyx.clear
to be executed. In the meantime, we could refresh the page and "sign in" again since an authToken is still stored.I took a quick look at the implementation of
Onyx.clear
, but I'm not sure how to improve its performance. Any suggestions? @kidroca @marcaaronThere 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.
yes and no, depending on what you consider a render
yes - it would trigger
render
method 3 times and also trigger any childrenrender
s.no - the screen might not re-render at all, or only do it once
setState is called per key and each time triggers a re-render
This would not make any subscribers aware of the change and
isAuthenticated
would not transition tofalse
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.
Yep, you're right.
OK, so another thought. We know that when we are signing out, we want everything to get unmounted. What if we unmount everything before calling
clear()
? Practically, that could be something like:Maybe that's still too much of a hack. Again, I'd rather fix the performance in the first place, but I did want to see what other alternatives we could think of.
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.
The best alternative (besides making clear or Onyx faster) is already applied
Removes
authToken
unmounting 95% of App and takes us to the SignIn pageThen Onyx.clear clears the rest of the stored values
@luacmartins Onyx.clear happens faster with these changes and doesn't take 2.3s, right?
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.
Hmm either something changed or my computer is faster today, as both cases took about 1.2-1.3s. The difference in timing is pretty small, so it's hard to say that clear happens faster because of those changes. I used the same account for both tests, cleared browsing data and did a fresh login in each scenario.
With changes
Without changes
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.
It seems JS blocking time is 100ms
Maybe in the past it was 1000 for some reason, or it was never the cause for the delay...
It might depend on the amount of data stored in index DB
Or if reads / writes were happening while clearing for some reason