react-native-onyx
react-native-onyx copied to clipboard
Expensify
`mergeCollection`: individual collection item subscriber is called even if the value did not change
When executing mergeCollection, Onyx calls the collection subscriber callback and all the collection items subscribers callbacks, even if those items did not change.
As a result, if there is a React component subscribed to a single collection item (e.g. a specific Report) and uses this item as a hook dependency (e.g. useEffect(..., [report])), the hook is executed every time the collection is merged, even if this specific item did not change.
Suggested solution: call individual collection item subscribers only if the item changes.
Example unit test that should succeed:
it('should not call a collection item subscriber if the value did not change', () => {
const connectionIDs = [];
const cat = `${ONYX_KEYS.COLLECTION.ANIMALS}cat`;
const dog = `${ONYX_KEYS.COLLECTION.ANIMALS}dog`;
const collectionCallback = jest.fn();
const catCallback = jest.fn();
const dogCallback = jest.fn();
connectionIDs.push(
Onyx.connect({
key: ONYX_KEYS.COLLECTION.ANIMALS,
callback: collectionCallback,
waitForCollectionCallback: true,
}),
);
connectionIDs.push(Onyx.connect({key: cat, callback: catCallback}));
connectionIDs.push(Onyx.connect({key: dog, callback: dogCallback}));
const initialValue = { name: 'Fluffy' };
const collectionToMerge = {
[cat]: initialValue,
[dog]: { name: 'Rex' },
};
return Onyx.set(cat, initialValue)
.then(() => {
Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ANIMALS, collectionToMerge);
return waitForPromisesToResolve();
})
.then(() => {
expect(collectionCallback).toHaveBeenCalledTimes(3);
expect(collectionCallback).toHaveBeenCalledWith({
[cat]: initialValue,
[dog]: { name: 'Rex' },
});
expect(catCallback).toHaveBeenCalledTimes(2);
expect(dogCallback).toHaveBeenCalledTimes(2);
Onyx.disconnect(connectionIDs);
});
});
I agree with the improvement you've suggested! It would make it work more consistently with the rest of the functions.
i can look into this over the next days, or if you want to work on this @paultsimura go ahead :)
Thanks @chrispader, I tagged you here on purpose in case you'd like to work on it 🙂 I'm quite busy with the C+ tasks at the moment 👍
Hey @chrispader, did you have any luck looking into this so far?
I believe this is the last issue blocking us from fixing the race condition in update operations.