mergeCollection: individual collection item subscriber is called even if the value did not change

[paultsimura]

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);
                    });
            });

[paultsimura]

@tgolen @chrispader WDYT about this issue?

[tgolen]

I agree with the improvement you've suggested! It would make it work more consistently with the rest of the functions.

[chrispader]

i can look into this over the next days, or if you want to work on this @paultsimura go ahead :)

[paultsimura]

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 👍

[chrispader]

Got it! I'll look into it

[paultsimura]

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.

[chrispader]

i can look into it today!

[chrispader]

Fixed in #541 :)

[paultsimura]

Thanks @chrispader 💪🏽

[chrispader]

I think we can close this :) @tgolen