Improve Ion.connect() only calls setState() once for each react component

[tgolen]

Referencing this cleanup task here: https://github.com/Expensify/ReactNativeChat/pull/426/files/aa194ecb6525a59748b57eccdaa16d0469ee4d26#diff-7cc9012ec34096901d5dc459cf9bd6c7R138

[tgolen]

@marcaaron Your original comment around this code was:

Here's the scenario I'm envisioning...

you have a regex key that matches on several Ion keys
we loop over each matched key, get the value, and call sendDataToConnection
sendDataToConnection calls setState() on the withIon component and uses the same state key each time
So I think what ends up on state will be the last matching key's value?

After inspecting the code this morning, I don't think this is accurate about what the code does. This code here:

https://github.com/Expensify/ReactNativeChat/blob/master/src/lib/Ion.js#L142-L147

will only call sendDataToConnection() once if there are multiple keys and the connection is using indexBy. The only case where sendDataToConnection() would be called more than once is if there is no indexBy, and that should be OK that it's undefined behavior because if you're subscribing to multiple keys with a React component, then you SHOULD be using indexBy. For now, I think maybe the best improvement would be to throw a warning in the code for the unsupported case.

[marcaaron]

Throwing an error for now is a great idea!

I'll try to add some context here since I'm not sure my point was understood and I think this is an opportunity to maybe simplify how this stuff works - but it's not particularly urgent.

1. When using indexBy we are be able to infer that we have a "collection" and we send the entire collection to the connection once on when we call Ion.connect(). This happens whether it's a component or a lib using Ion.connect() directly. But when keyChange() is called for a matching key it seems we only consistently build the collection for the React component case and not the callback. I'm not sure there are any places where we are using indexBy with a direct subscriber, but it's something to watch out for since the first payload will be the collection and all later updates will be individual records.

2. When we are not using indexBy the regex we pass as a key could match on one or more keys. It sounds like we agree that this shouldn't be supported. When we first call Ion.connect() we don't merge the results of those two values together in any way we're just quickly update one after the other. This makes sense for the callback case since we can call a library subscriber callback (anything calling Ion.connect() directly) multiple times for each key we find and return each record one by one. However, for the React component case we just quickly overwrite the value.

So, nothing is broken yet. But I wonder if we can make this more predictable somehow.

My first instinct is to say that regex keys are really cool, but we might not need them at all. The main use case seems to be for when we have a "collection". So we could maybe double down on the construct of a "collection" and make it more easily identifiable (vs. something that we put together in our heads when we see the combination of indexBy + key: 'SOMETHING_%DATAFROMPROPS%').

My proposal to improve this would be to remove all the cases where we have regex keys and instead be very specific about what we want to connect to... it's either going to be a single record (${IONKEYS.COLLECTION.REPORT}_${reportID}) or a collection (IONKEYS.COLLECTION.REPORT). We can then add some logic to Ion so that it's able to identify based on the key that we are dealing with a collection or non collection key. We won't need an indexBy since the indexBy is just whatever comes after the _ in the key name.

Anyway, that's my idea. Maybe there are other use cases for the regex keys that I'm missing the context for (e.g. maybe we will have two completely different data types that we'll want to merge into a single collection that will be indexed by a property name).

[tgolen]

Spoke 1:1 with Marc about this to throw out some other ideas. We landed on this being a great premise to start with:

Every time time data is changed in Ion (wether it happens in Ion.connect() as part of the initialization code, or in keyChanged() when a value changes), all subscribers should get the same predictable data format regardless if it's subscribing to a single key or multiple keys, and regardless if it's a callback or a React component

In this universe, that would essentially treat every subscription like it's a collection, and there could be collections with single keys, or collections with multiple keys, but everything is a collection.

[tgolen]

I'm going to close this because this has mostly been refactored for now.