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

Clear cache in Onyx.clear() #245

Merged
merged 3 commits into from
Apr 24, 2023
Merged

Clear cache in Onyx.clear() #245

merged 3 commits into from
Apr 24, 2023

Conversation

roryabraham
Copy link
Contributor

@roryabraham roryabraham commented Apr 24, 2023

Details

We recently updated Onyx.clear such that it actually removes items we don't want to keep from storage, rather than just setting them to null.

However, we did not update Onyx.clear() to update the cache accordingly – it still just sets items to null. I think this is expected for any existing Onyx subscribers – the value should go from being whatever it was before to null. However, any new call to Onyx.connect should not have any record of cleared keys.

cc @tienifr

Related Issues

https://github.com/Expensify/Expensify/issues/277227 – related to tests

Automated Tests

I was testing this in Expensify/App#17718 – there are currently some TODOs above code that filters out null items from Onyx because they're making it harder to make assertions about things like the number of reports in Onyx.

After updating Onyx to use the latest version which includes the new version of Onyx.clear and removing the filters, I found that the test was still failing and showed reports that should have been cleared as null in Onyx. After debugging, I discovered that the problem was that the cache was not being updated correctly.

I also updated the tests in this repo accordingly, and I think that the new behavior is correct.

Linked PRs

Expensify/App#17718

@roryabraham roryabraham self-assigned this Apr 24, 2023
@roryabraham roryabraham requested a review from a team as a code owner April 24, 2023 16:17
@melvin-bot melvin-bot bot requested review from chiragsalian and removed request for a team April 24, 2023 16:18
@roryabraham roryabraham changed the title Clear cache in Onyx.clear() [WIP] Clear cache in Onyx.clear() Apr 24, 2023
@roryabraham
Copy link
Contributor Author

Edit: We were updating the cache to null, and that's kind of necessary to inform the subscribers that the data has dropped. Need to think about this some more

@roryabraham roryabraham changed the title [WIP] Clear cache in Onyx.clear() Clear cache in Onyx.clear() Apr 24, 2023
Copy link
Contributor

@chiragsalian chiragsalian left a comment

Choose a reason for hiding this comment

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

lgtm

@chiragsalian chiragsalian merged commit 4137691 into main Apr 24, 2023
@melvin-bot melvin-bot bot added the Emergency label Apr 24, 2023
@melvin-bot
Copy link

melvin-bot bot commented Apr 24, 2023

@chiragsalian looks like this was merged without a test passing. Please add a note explaining why this was done and remove the Emergency label if this is not an emergency.

@chiragsalian
Copy link
Contributor

Tests were passing just fine.

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.

2 participants