Skip to content
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

Allow clear in onyx update #156

Merged
merged 3 commits into from
Jul 8, 2022
Merged

Allow clear in onyx update #156

merged 3 commits into from
Jul 8, 2022

Conversation

Beamanator
Copy link
Contributor

@Beamanator Beamanator commented Jul 8, 2022

cc @marcaaron @Julesssss @Justicea83 since y'all are reviewing the related Web-E PR 👍

Details

As noted by @marcaaron here, we use Onyx.clear() when signing out of NewDot. This is fine but in the recent API refactor we need to call Onyx.clear in a few places after closing an account (the related PRs & issue) & maybe even after signing in

Currently we do Onyx.clear().then because Onyx.set is faster than Onyx.clear. We won't have to use .then if we use Onyx.merge though b/c Onyx.merge is slower than Onyx.clear

  • Investigation here: Set default key states when clearing the cache #129 (comment)
  • Comments in code here:
    * Note that calling Onyx.clear() and then Onyx.set() on a key with a default
    * key state may store an unexpected value in Storage.
    *
    * E.g.
    * Onyx.clear();
    * Onyx.set(ONYXKEYS.DEFAULT_KEY, 'default');
    * Storage.getItem(ONYXKEYS.DEFAULT_KEY)
    * .then((storedValue) => console.log(storedValue));
    * null is logged instead of the expected 'default'
    *
    * Onyx.set() might call Storage.setItem() before Onyx.clear() calls
    * Storage.setItem(). Use Onyx.merge() instead if possible. Onyx.merge() calls
    * Onyx.get(key) before calling Storage.setItem() via Onyx.set().
    * Storage.setItem() from Onyx.clear() will have already finished and the merged
    * value will be saved to storage after the default value.

Related Issues

https://github.com/Expensify/Expensify/issues/213701

Automated Tests

Not needed, just adding the ability to have clear be an onyxMethod

Note: This PR was tested in combination with:

Results here: https://github.com/Expensify/Web-Expensify/pull/34159#issuecomment-1179061881

Linked PRs

https://github.com/Expensify/App/pull/9635/files

@Beamanator Beamanator self-assigned this Jul 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2022

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Beamanator
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@Beamanator Beamanator marked this pull request as ready for review July 8, 2022 14:43
@Beamanator Beamanator requested a review from a team as a code owner July 8, 2022 14:43
@melvin-bot melvin-bot bot requested review from Justicea83 and removed request for a team July 8, 2022 14:44
@Beamanator Beamanator requested a review from marcaaron July 8, 2022 14:44
Copy link
Contributor

@Justicea83 Justicea83 left a comment

Choose a reason for hiding this comment

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

Code looks pretty straight forward to me

@Julesssss Julesssss merged commit 53a49c6 into main Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants