Make Onyx.getAllKeys public

[Gonals]

@alex-mechler 🐻

cc @marcaaron to make sure I'm not doing anything that goes against our standards.

Details

For Expensify/App#4453, I needed the ability to, basically, replace a whole collection with a new one. Since we currently don't have an easy way of doing so, I need to retrieve all existing keys so I can decide which ones to clear.

Related Issues

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

Automated Tests

None

Linked PRs

Expensify/App#4453

[github-actions]

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

[Gonals]

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

[marcaaron]

I needed the ability to, basically, replace a whole collection with a new one

Hmm can you maybe explain what you are trying to do exactly? If we want to remove an item from a collection we can remove the key for that item from Onyx. Collections are aggregates of many individual keys.

[marcaaron]

I'm not sure I agree with adding this functionality to mergeCollection().

In Onyx, "merge" is supposed to merge a new value (usually an object) into an older one. If the new value is missing a key that the old value has I wouldn't expect that it would be removed at all (even with the optional flag). So it feels like mergeCollection() should follow that same pattern.

If we need to remove a key from Onyx we should maybe just allow explicitly removing a key instead of having it happen in the mergeCollection()? Looks like Onyx.remove() is not a public method, but I'm not sure why it shouldn't be?

Could that work ?

[alex-mechler]

This is a good point, I've cleared my approval because I agree that this isnt something merge should be doing

[Gonals]

Yep, I did address this in my first comment. My reasoning behind putting it here was to avoid the alternative, which was creating a new method (setCollection) that would basically first call mergeCollection, and then, again, get AllKeys and do the new functionality. Would that work?

If we need to remove a key from Onyx we should maybe just allow explicitly removing a key instead of having it happen in the mergeCollection()? Looks like Onyx.remove() is not a public method, but I'm not sure why it shouldn't be?

Removing the key is setting it to null, right? That is basically what the new code does for all keys in a collection that are not in the "new" collection passed. I think it is a useful method to have for a number of things. Basically, any collection where we can remove elements in the backend could benefit from this.
We can totally replicate the functionality by manually deleting the keys in each case, but why would we?

[Gonals]

I've moved the logic to a new function: setCollection. LMK what you think

[Gonals]

I needed the ability to, basically, replace a whole collection with a new one

Hmm can you maybe explain what you are trying to do exactly? If we want to remove an item from a collection we can remove the key for that item from Onyx. Collections are aggregates of many individual keys.

Yep, that is correct (and it is what the code does). This is just a helper function.
My current flow is:

• We have a list of policies in the backend.
• We load them to onyx as a collection. We have them there.
• Some of those policies get deleted.
• We load them to onyx as a collection. We update any existing ones and add the new ones, but the deleted ones are not removed.
• This functionality finds those and removes them.

I could do it manually for this specific case, but it sounded like a good helper function to have in the future for similar cases.
Thoughts?

[marcaaron]

I think we can just see which keys exist by subscribing to the collection? If the new value that comes back from the API is different then what we have here then just remove those keys?

[Gonals]

Ah, very true. Closing this PR, since we no longer need it. Thanks for all the input!