More improvements for Onyx.connect (After cache)

[kidroca]

There are some more improvements that can be done to the connect function specifically for storage accessing

1. Improvements regarding the getAllKeys usage in connect

react-native-onyx/lib/Onyx.js

Lines 373 to 382 in 27c0ba8

	getAllKeys()
	.then((keys) => {
	// Find all the keys matched by the config key
	const matchingKeys = _.filter(keys, key => isKeyMatch(mapping.key, key));
	// If the key being connected to does not exist, initialize the value with null
	if (matchingKeys.length === 0) {
	sendDataToConnection(mapping, null);
	return;
	}

It serves to distinct any collection keys when connecting to a collection - something that ATM is not possible without first retrieving all storage keys
The above wasn't easy to tell by the code itself, I only find out when debugging

So we only need getAllKeys when we're dealing with a collection to get a list of collection item keys - matchingKeys
Otherwise we're always connecting to a single key - mapping.key

An oversimplified example

let matchingKeys = [mapping.key];
if (isCollectionKey(mapping.key)) {
  const allKeys = await getAllKeys();
  matchingKeys = _.filter(allKeys, key => isKeyMatch(mapping.key, key));
}

2. Values can be retrieved using multiGet

We already have a list of keys and we can use multiGet instead of retrieving each key separately

react-native-onyx/lib/Onyx.js

Lines 389 to 401 in 27c0ba8

	if (mapping.withOnyxInstance && isCollectionKey(mapping.key)) {
	Promise.all(_.map(matchingKeys, key => get(key)))
	.then(values => _.reduce(values, (finalObject, value, i) => ({
	...finalObject,
	[matchingKeys[i]]: value,
	}), {}))
	.then(val => sendDataToConnection(mapping, val));
	} else {
	_.each(matchingKeys, (key) => {
	get(key).then(val => sendDataToConnection(mapping, val, key));
	});
	}
	});

This can instead be:

// Result is like: [['k1', 'val1'], ['k2', 'val2']]
const retrievedKeyValuePairs = await multiGet(matchingKeys);
  
if (mapping.withOnyxInstance && isCollectionKey(mapping.key)) {
  const asCollection = _.object(retrievedKeyValuePairs); // see https://underscorejs.org/#object
  sendDataToConnection(mapping, asCollection));
}
else {
  _.each(retrievedKeyValuePairs, ([key, val]) => sendDataToConnection(mapping, val, key));
}

Currently there's no multiGet definition in Onyx, but we can add one and hook it to cache as well.
It can check if any of the keys are available in cache and only call AsyncStorage.multiGet with the remaining, then cache each key/val from the result

As we've seen from previous benchmarks: Expensify/App#2762 (comment) retriving multiple items in parallel even with cache is flooding the bridge with calls (when data is not available in cache e.g. startup)

[kidroca]

Onyx.connect called for a collection

Onyx.connect called for a single item

[quinthar]

Very cool! I love this!

[marcaaron]

@kidroca should we maybe break these 2 issues up into individual issues to focus on in isolation?
Also, is "Values can be retrieved using multiGet" somehow different from "[Optimization] Easy Onyx.get batching"?

[kidroca]

@kidroca should we maybe break these 2 issues up into individual issues to focus on in isolation?

The first part of the ticket might not be necessary, the result of getAllKeys comes from cache ever since the first usage so it works quick

Also, is "Values can be retrieved using multiGet" somehow different from "[Optimization] Easy Onyx.get batching"?

Hmm yeah they are similar, but the idea here is to use multiGet since we have a list of items matchingKeys. While the other ticket #84 proposes to use multiGet for every get (e.g. the once happening internally as well) because AsyncStorage.multiGet would batch keys with setImmediate

We can probably close this ticket and keep the newer one

[marcaaron]

Alright sounds good. I'm going to close up this issue then. It would be great if we can pull out some of these observations and focus on each one in isolation. I'm trying to help funnel these ideas into actionable steps but having issues like "optimize x" and "improve y" make it pretty tricky to tell what is what.

[marcaaron]

Also, let's move these issue into Expensify.cash please so they can get more attention :)