Add waitForCollectionCallback to keysChanged

[luacmartins]

cc @tgolen

Details

In #163, we added a new param waitForCollectionCallback to Onyx.connect that waits for the collection to be read and triggers the callback only once. However, we forgot to add that param to keysChanged which means that the callback is being triggered for each key when we update the collection.

This PR fixes that and ensures that the callback is called only once when we update the collection.

Related Issues

#163

Automated Tests

Added in onyxTests

Linked PRs

Expensify/App#10051

[tgolen]

Hm, maybe this test can be split up better for testing, because it's becoming a bit unclear to me what it's doing. Each test should have a very clear GIVEN/WHEN/THEN, but right now this has GIVEN/AND/WHEN/THEN/WHEN/THEN/AND 😅

Could you try making the test more clear to test each case independently?

[luacmartins]

Sure thing! I split the test into two, one to test Onyx.connect with initial data and one to test updating a collection after we are connected to it!

Ready for another review!

[aldo-expensify]

lol, ignore my first deleted comment, I missed the return

[aldo-expensify]

Left 2 small comments, overall looks good!

[luacmartins]

Updated!

[aldo-expensify]

I'll leave the merge to @tgolen so he can review

[tgolen]

Those tests looks great now! Thank you

I wanted to question some assumptions, and my disclaimer is, it's been a long time since I've worked on that connect() code, so I don't fully remember what's feasible or not.

[tgolen]

Does it seem weird to anyone else that:

1. There would be two calls
2. The first call passes null
3. The second call is really what we want with waitForCollectionCallback=true

?

Is it possible for us to remove that first call? What is the value in having the call triggered with null before the collection has been "initialized"?

[luacmartins]

I think this is expected since we want to initialize that key's value to null, since we are connecting to that key, we just don't have a value for it. We leave it up to the subscriber to decide what to do with a null value.

This happens here before we run any callback subscriber logic. I think it would make sense to keep the behavior consistent.

[aldo-expensify]

+1 to what @luacmartins said :), I would also prefer to avoid exceptions and let the subscriber handle that. What if null is a valid value and we want to be notified about it?

[tgolen]

Yeah, those are good points. I think it would be a change in behavior that could make other bugs in the process, so this was more of an exploratory question. Let's leave it as-is!

[marcaaron]

In #163, we added a new param waitForCollectionCallback to Onyx.connect that waits for the collection to be read and triggers the callback only once

I'm kind of confused about this decision though I understand the need that it fills and appreciate the improvement.

It almost feels like this should be the default behavior based on how subscribing to a collection key works when there is a withOnyx instance.

Just curious if anyone agrees that it would be more consistent to have something like:

Onyx.connect({key: ONYXKEYS.COLLECTION.REPORT}, callback: (allReports) => {...})

because that's what this code would do:

withOnyx({
    allReports: {key: ONYXKEYS.COLLECTION.REPORT},
})(MyComponent);

And if we want individual member values returned then we should do something like

Onyx.connectToCollectionMembers({key: ONYXKEYS.COLLECTION.REPORT, callback: (value, key) => {...}});

Last, I'm just not really sure about the name waitForCollectionCallback. What are we waiting for exactly? 🤔

[marcaaron]

Also unsure if this is known (and apologies in advance if I'm wrong about this) but the data returned changes based on whether mergeCollection or merge is used...

keyChanged() does this and won't look at the optional flag:

react-native-onyx/lib/Onyx.js

Lines 330 to 332 in 1f58aa5

	if (_.isFunction(subscriber.callback)) {
	subscriber.callback(data, key);
	}

But keysChanged() (triggered when mergeCollection is used) will always return the collection

react-native-onyx/lib/Onyx.js

Lines 260 to 267 in 1f58aa5

	if (isSubscribedToCollectionKey) {
	if (_.isFunction(subscriber.callback)) {
	const cachedCollection = getCachedCollection(collectionKey);
	if (subscriber.waitForCollectionCallback) {
	subscriber.callback(cachedCollection);
	return;
	}

Kind of unpredictable, no? How do you know which method is used and what to expect in the callback?

[tgolen]

What are we waiting for exactly?

For the entire collection to be read out of Onyx into memory.

@marcaaron Can you clarify what you are proposing? It sounds like you're implying some kind of proposal (which looks good to me so far), but I'm just not clear what you're suggesting.

That's a good point about merge vs mergeCollection.

[marcaaron]

I'm proposing that we make the behavior of Onyx.connect() and withOnyx for "collections" the same i.e. when you connect directly to a connection key it should return the entire updated collection when:

• The initial connection happens
• Any of the collection member keys update

That would be more consistent and not require us to use a flag like waitForCollectionCallback which doesn't describe the behavior that it enables well (when and why should we "wait"? what exactly is the "collection callback"?). This is the default behavior for withOnyx, but we have to opt-in when using Onyx.connect().

[tgolen]

Ah, got it. That makes sense to me 👍

[luacmartins]

I agree with your proposed solution @marcaaron! Let's do it!