[Fix #269] use cached data

[hannojg]

Details

This PR introduces support for withOnyx to load data from cache immediately and synchronously. This greatly increases the chance when rending a withOnyx wrapped component that it can render right away, instead of being blocked by a loading state at first.

Overview of the changes made:

• tryGetCachedValue: New method in Onyx.js that is used by withOnyx to get values from cache
• withOnyx.js: In the constructor of the wrapper class try to load all data from cache. Only activate loading state, if not all data was available from cache.
• Onyx.js:
  • Cleaned up the code a bit by combining certain statements
  • Added a few more equality checks before calling setState

Related Issues

#269

Automated Tests

The tests have been modified to reflect the changes made. In particular a lot of the waitForPromiseToResolve calls were removed. They aren't needed anymore, as a component can render immediately, once we have the data in onyx, as we don't need to wait for the next tick.

Linked PRs

/

[kidroca]

Thank you for your efforts on this pull request. Overall, the changes look promising and I appreciate the thoughtfulness behind them.

I've shared some specific comments for your consideration to improve the overall quality and maintainability of the code.

One particular observation is that there seem to be some changes in this PR that aren't directly tied to the referenced issue. While these changes, which appear to be performance optimizations and refactoring, could indeed enhance our codebase, they also introduce extra complexity to the review process.

To streamline the review, I suggest we separate these optimizations and refactoring changes into a subsequent PR. This approach would allow us to maintain focus on addressing the primary objective of the current PR, and may even speed up its approval.

Looking forward to your thoughts. Thanks again for your contributions!

[kidroca]

I've noticed that our coding standard promotes the use of early returns to streamline control flow. Although the current nesting may have caused eslint to miss this, I believe we should still adhere to this practice. Particularly, the if (_.keys(values).length > 0) { return values; } statement seems a good candidate to be moved up for an early exit if there are no matchingKeys.

Is there a valid scenario where a collection key would need to progress through the rest of the logic without any matchingKeys? This might lead to unintended side effects. For instance, if the code execution reaches line 227 val = cache.getValue(key) without any keys, the returned val is likely to be undefined. If there's no compelling reason for this behaviour, it might be beneficial to return early to avoid unnecessary operations and potential ambiguity in the function's behaviour.

[hannojg]

Hey, good point, I updated the logic. When we didn't find any values for the collection key we shouldn't continue with the functions behaviour. So I added an early return statement for that.

[Julesssss]

Great work, would love to see a bit more thorough comments for people like me who don't fully understand the logic.

[hannojg]

Hey, thanks for the first quick reviews! I will do as @kidroca said, and make a separate PR for the code cleanings / refactoring to make sure the review process is simpler.

Will put it again into draft until that happened. Please only continue reviewing, once the PR is converted back to normal, thanks! 😊

[hannojg]

Good for review again. I was also experimenting with removing the if(loading) return null logic, but that needs a bit more thorough testing.

[tgolen]

Looks great, thank you! Just this one last typo.

[luacmartins]

Looking good. Left a couple of small comments

[marcaaron]

Looks good, but I have some concern about using the defaultKeyStates as a cache value fallback. I think this behavior is wrong.

[mountiny]

Thanks for pushing this forwards!

[mountiny]

I am not sure why do we set the tempState to the same cachedState as the normal state. I might be missing something but could you help me understand this? Should we update the comment above?

[marcaaron]

The intention here is that tempState is still used in the same way as it was before.

If we did not get all the values from the cache then the values are held in the tempState until all are "loaded". This way we avoid calling setState() each time a value comes back from storage. That was the original optimization added here and is a good thing because otherwise you may have like several calls to setState() happening in a kind of burst before the first render (at least that's what we saw and why we changed how this works).

Though the question did make me wonder about something... if this.tempState and this.state are initialized with the same object reference then will we be mutating the state directly on this line:

react-native-onyx/lib/withOnyx.js

Line 117 in 80c6f00

this.tempState[statePropertyName] = val;

Seems like that would be undesirable in the case where you did not initialize all the values in the constructor.

[mountiny]

Thanks for the write up. @hannojg what do you think of the concern Marc raised above?

Concerns addressed

[mountiny]

Per the slack discussion we got enough approvals here and we want to get this change out to test how it improves performance so I am going to merge this https://expensify.slack.com/archives/C035J5C9FAP/p1690473404613159?thread_ts=1690290738.145189&cid=C035J5C9FAP

[marcaaron]

haven't tested the theory, but this might be a problem -> #280 (comment)