Skip to content

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.

4 participants