Add initialValue, delayUpdates to allow components to eagerly render. #329

ospfranco · 2023-09-05T12:36:10Z

Details

Margelo is currently with improvements to the ReportScreen speed. After thorough analysis a big bottleneck came from react-native-onyx itself. Components with a lot of connections can cause frequent re-renders or delay the rendering of the component itself until all data has been fetched/hydrated.

We had to implement several changes in order to get the rendering speed down:

Add initialValue to a key. Allows to pass a initial value while the data is being fetched, thus allowing the component to mount/render ASAP.
Added delayUpdates to a withOnyx connection. Allows to delay setting updates/state of the withOnyx hook to allow the component to mount first and then update in a single batch at a different time. The child component gets passed a allowUpdates function that it can use to tell the withOnyx HOC to start applying the batched updates.
Another PR has been created that brings back a reverted improvement made by margelo on collection keys. This also has a big impact on this performance improvements.

Related Issues

Part of Expensify/App#26087

Automated Tests

Linked PRs

github-actions · 2023-09-05T12:36:28Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

ospfranco · 2023-09-06T05:12:29Z

I have read the CLA Document and I hereby sign the CLA

yuwenmemon

Can we undo the whitespace and quotation changes so this PR is a lot less noisy?

also cc @marcaaron and @tgolen who might be interested in having a peek at this.

yuwenmemon · 2023-09-06T17:21:52Z

lib/Onyx.js

-import fastMerge from './fastMerge';
-import * as PerformanceUtils from './metrics/PerformanceUtils';
-import Storage from './storage';
+import { deepEqual } from "fast-equals";


A lot of updates here that deviate from our style guide. Can we re-update to get these back to that standard?

Ah, I just left prettier do it's thing, what is the configuration I should use?

I cleaned all the formatting changes, should be a lot let noisy now

yuwenmemon · 2023-09-06T17:24:17Z

lib/Onyx.js

+const getSubsetOfData = (sourceData, selector, withOnyxInstanceState) =>
+    selector(sourceData, withOnyxInstanceState);


I also question if updates like this adding new lines for lines that weren't already that long are necessary. They're adding a lot of noise to this PR IMO.

I cleaned all the formatting changes, should be a lot let noisy now

yuwenmemon · 2023-09-06T17:30:23Z

lib/utils.js

@@ -0,0 +1,3 @@
+export function areObjectsEmpty(a, b) {
+  return typeof data === 'object' && typeof previousData === 'object' && Object.values(a).length === 0 && Object.values(b).length === 0


Where do data and previous data come from and what is this method achieving exactly?

You are right, this was a copy and paste mistake from my other PR, the correct values are a and b

marcaaron · 2023-09-06T19:25:08Z

Sorry I have been reviewing a lot of Onyx PRs and can't prioritize a review today as these changes looks quite involved. Also +1 to @yuwenmemon's comment about formatting. It looks like prettier ran with the wrong settings maybe.

First, thanks for the work @ospfranco. Excited to discuss everything with you. I will leave some questions and suggestions regarding the PR description as there are some things I'm not really clear on and wouldn't really be able to review the changes without understanding them first.

After thorough analysis a big bottleneck came from react-native-onyx itself

Where exactly was the bottleneck and what did the analysis show? Please add this to the description.

Components with a lot of connections can cause frequent re-renders

Yes, if you have a lot of subscribers then they will re-render when there is a change. Is there some specific example of this that showcases the problem better? Did we discuss any alternatives?

or delay the rendering of the component itself until all data has been fetched/hydrated.

@hannojg's PR to use the cached values was helping with this part. Is it not enough? Where does this delay happen? What does it feel like?

Add initialValue to a key. Allows to pass a initial value while the data is being fetched, thus allowing the component to mount/render ASAP.

Any discussion to link? I am not sure if it's clear that we want or need anything like an "initial value" to be added to the Onyx API.

Added delayUpdates to a withOnyx connection. Allows to delay setting updates/state of the withOnyx hook to allow the component to mount first and then update in a single batch at a different time. The child component gets passed a allowUpdates function that it can use to tell the withOnyx HOC to start applying the batched updates.

Is the hook something we already have or are adding here? I didn't see it in the PR but only looked briefly. Overall, this idea sounds kind of neat, but also sounds like added complexity. Also, this might be semantics, but I think delayUpdates is ambiguous as the rendering of the component is currently "delayed" (we wait for all the values to be retrieved). Maybe we can find clearer language to use around that - I'll think about it.

#330 has been created that brings back a reverted improvement made by margelo on collection keys. This also has a big impact on this performance improvements.

Is this related to the current PR?

ospfranco · 2023-09-07T07:51:54Z

@marcaaron

After thorough analysis a big bottleneck came from react-native-onyx itself

Where exactly was the bottleneck and what did the analysis show? Please add this to the description.

Components with a lot of connections are too slow, e.g. ReportScreen, this container grew and is now connected to a lot of entities. As onyx updates the component state via HOC, more updates are fetched from the DB or API responses come back, this causes the component to be constantly re-render, sometimes it delays the initial rendering and almost always causes subsequent re-renders. In the case of the ReportScreen being this a container on top of the tree, each re-render is very expensive and delays everything, from screen transition animation, to the list of actions rendering.

Components with a lot of connections can cause frequent re-renders

Yes, if you have a lot of subscribers then they will re-render when there is a change. Is there some specific example of this that showcases the problem better? Did we discuss any alternatives?

All the changes were done when testing the ReportScreen and that is the best example. You can see on this line on the PR on expensify, one solution we had to come up to keep the animation running smoothly is to delay the update of the component, after is has mounted and has been layed out, then allow onyx to start the many queued state changes of all the data is has pending.

The only other solution we considered might have an impact was the batching of updates, as it avoid frequent re-renders by batching the updates, but that PR/discussion is still pending.

or delay the rendering of the component itself until all data has been fetched/hydrated.

@hannojg's PR to use the cached values was helping with this part. Is it not enough? Where does this delay happen? What does it feel like?

No, it is not enough, once values are in cache it surely does help, but on first mount/render and on a large-subtree with lots connections it causes delays in many components, in total delaying the transition/first render. By adding the initialValue prop, we can allow the component to render as fast as possible, while Onyx is fetching stuff from DB. The state might not be always consistent with the data, but it is rather not an issue for most components. Again, the best example is the entire ReportScreen rendering that hang-ups while the many re-renders ocurr.

Add initialValue to a key. Allows to pass a initial value while the data is being fetched, thus allowing the component to mount/render ASAP.

Any discussion to link? I am not sure if it's clear that we want or need anything like an "initial value" to be added to the Onyx API.

No, all the discussions were internal. Here is an example of a Performance trace in flipper:

It's hard to explain, but basically, the first delay, plus frequent sub-sequent state updates simply do not allow React to flush and therefore delay everything until everything settles down.

Added delayUpdates to a withOnyx connection. Allows to delay setting updates/state of the withOnyx hook to allow the component to mount first and then update in a single batch at a different time. The child component gets passed a allowUpdates function that it can use to tell the withOnyx HOC to start applying the batched updates.

Is the hook something we already have or are adding here? I didn't see it in the PR but only looked briefly. Overall, this idea sounds kind of neat, but also sounds like added complexity. Also, this might be semantics, but I think delayUpdates is ambiguous as the rendering of the component is currently "delayed" (we wait for all the values to be retrieved). Maybe we can find clearer language to use around that - I'll think about it.

It is something added. I cleaned the PR, it should be easier to spot now. You can remove it, but then you will always have the same problem as components grow over time. Fetching from DB/API will not allow them to mount on first render and screen transitions/mounting will be delayed until onyx reaches a stable state.

#330 has been created that brings back a reverted improvement made by margelo on collection keys. This also has a big impact on this performance improvements.

Is this related to the current PR?

Yes, it does have a big impact on screen transitions.

Just to further make the point, here is the ReportScreen transition, before and after hour changes:

Most of the performance gains came from adding initialValues to many smaller components and the delayUpdate of the big container:

IMG_9525.MOV

IMG_9526.MOV

Linting on utils

tgolen · 2023-09-07T15:30:01Z

Thanks for all that information.

Here is an example of a Performance trace in flipper:

Can you please add before/after traces so it can be directly compared? Another thing that would be useful is specific timings for what "delayed" means. Is it seconds? milliseconds? microseconds?

When we do anything related to performance there should always be measurements of before and after to quantify and demonstrate the improvement.

Most of the performance gains came from adding initialValues to many smaller components

Could you please link to a PR (it's fine if it's only a draft) of App with these changes so I can see what the initialValues change actually looks like?

tgolen · 2023-09-07T15:31:55Z

lib/utils.js

+    return (
+        typeof a === 'object'
+        && typeof b === 'object'
+        && _.values(a).length === 0


What's the difference between this and using _. isEmpty_() or _.size() === 0?

Initial implementation was Object.values(a).length, linter barked at me I should use underscore, no other reason. I will change it to _.isEmpty

tgolen · 2023-09-07T15:32:37Z

lib/utils.js

@@ -0,0 +1,12 @@
+import * as _ from 'underscore';


Now that there is a utils.js file, is there more stuff that can be moved here? Maybe the fast merge stuff?

Yeah, makes sense.

tgolen · 2023-09-07T15:33:40Z

lib/utils.js

@@ -0,0 +1,12 @@
+import * as _ from 'underscore';
+
+function areObjectsEmpty(a, b) {


I think this implementation is too specific in that it requires two objects. I think there are already existing methods to check if a single object is empty, and then it's just as easy as doing if (_.isEmpty(a) && _.isEmpty(b)) and then it makes this utility method go away.

_.isEmpty(8) is giving me true. Because we don't know what Onyx is sending to the connection, we only really want to do this check to avoid unnecessary re-renders on objects (and maybe arrays? I didn't see any array while debugging)

There are some arrays, but they are less frequently used.

This one comes to mind: https://github.com/Expensify/App/blob/48437acb34dbc38b9b7a60dae4c272727b8ce3ab/src/libs/actions/ActiveClients.js#L9

tgolen · 2023-09-07T15:37:01Z

lib/withOnyx.js

            render() {
+                // Remove any null values so that React replaces them with default props
+                const propsToPass = _.omit(this.props, value => _.isNull(value));


Does this work?

Suggested change

const propsToPass = _.omit(this.props, value => _.isNull(value));

const propsToPass = _.omit(this.props, _.isNullvalue);

only passing _.isNull should work

lib/withOnyx.js

tgolen · 2023-09-07T15:39:18Z

lib/withOnyx.js

+                        const key = Str.result(mapping.key, props);
+                        let value = Onyx.tryGetCachedValue(key, mapping);
+                        if (!value && mapping.initialValue) {
+                            value = mapping.initialValue;


Is there any way to just get the default value out of the existing defaultProps? It feels very redundant to me to have both.

That's a good idea, I'm not sure, but I will take a look

I'm thinking about this, and I'm not sure it is the best idea. I really prefer explicit behaviors to implicit ones. By applying this change all the loading states from components that use the withOnyx hook will change, and probably no loading states will be shown any more.

FWIW if we end up implementing a hook for Onyx then we don't really need defaultProps at all as the values are not props anymore. The interaction becomes simpler because you can:

Stop using withOnyx()

Delay the rendering (or not) at your discretion

No more defaultProps

// I am not a prop. Find some other way to validate my type 🙃 const {value: allReports} = useOnyx({key: 'reports_', initialValue: {}}); if (empty(allReports)) { return null; }

oh yea, also, the TS migration appears to have proposed we do away with defaultProps entirely

Without the hook though - to me, this begs the question of whether the initialValue should later be restored if the value becomes undefined in the future. It's an "initial value", but easy to confuse it as a "default value".

Should I still provide a default via props or default props? In this case, yes. It is only used for setting the initial value.

marcaaron · 2023-09-07T22:10:07Z

Thanks for the writeup @ospfranco !

This might be a naive interpretation of the problem, but we can reduce it to:

it takes time to rehydrate state from storage and this blocks rendering

It looks like these delays are the cost of loading from storage into memory lazily. So, one thought that comes to mind is whether we can solve this by prioritizing the work to hydrate the state earlier?

If the app was burdened with less data requirements and could rehydrate the critical state faster - would we still need this solution?

I am curious about proceeding here without an answer to that question though not trying to block on this if @yuwenmemon and @tgolen feel more confident than me.

From what I can tell the interactions will be faster, but the UX also janky because we load an initial value -> stored value later -> maybe API stuff later. Probably we will want to smooth that out with a loading state of some kind, but having faster interactions is a net positive change.

yuwenmemon

A few more notes to reduce noise even more

lib/Onyx.js

lib/withOnyx.js

ospfranco · 2023-09-08T14:38:16Z

@marcaaron pre-loading/pre-fetching is a solution, but it is not always applicable or practical. For example, action reactions are not really linked to anything and it is not possible to know before hand which ones can be pre-loaded. Other entities might require loading the entire db into memory to be really effective. In any case, this is rather a techinique for further optimization and the current implemented solution allows for a more generic solution, which is allowing the components to render while state is being fetched.

…pdates

ospfranco · 2023-09-08T16:16:22Z

@tgolen

Thanks for all that information.

Here is an example of a Performance trace in flipper:

Can you please add before/after traces so it can be directly compared? Another thing that would be useful is specific timings for what "delayed" means. Is it seconds? milliseconds? microseconds?

Here is the trace with all the performance optimizations:

Everything is in milliseconds

When we do anything related to performance there should always be measurements of before and after to quantify and demonstrate the improvement.

Most of the performance gains came from adding initialValues to many smaller components

Could you please link to a PR (it's fine if it's only a draft) of App with these changes so I can see what the initialValues change actually looks like?

Here is the related PR

Expensify/App#26772

marcaaron · 2023-09-08T20:19:05Z

Thanks for continuing the discussion with me @ospfranco!

pre-loading/pre-fetching is a solution, but it is not always applicable or practical.
For example, action reactions are not really linked to anything and it is not possible to know before hand which ones can be pre-loaded.

The reactions are something we could prefetch. They are linked to the actions via reportActionID, the actions are linked to the report via reportID, the reports are shown in the sidebar (and sometimes other places). We can prefetch the needed data on layout, scroll, hover, etc.

We could also layer something like this over your proposed changes later to improve the UX so I'd love to hear your thoughts on this idea either way.

Which data are we trying to load specifically that is causing a delay? Is it all stuff we need? Why is it slow? Can we do anything to load it faster?

Other entities might require loading the entire db into memory to be really effective.

Which entity? I feel like we are talking about some hypothetical mega component that for some reason needs the entire database in memory and actually can't think of what that would be.

In any case, this is rather a techinique for further optimization and the current implemented solution allows for a more generic solution, which is allowing the components to render while state is being fetched.

Overall, this looks like a good idea that we probably need. I'm just trying to understand if we need it right now. It being a "generic solution" is part of my concern. Generic solutions sometimes add complexity or cause regressions for not much obvious benefit.

It does also feel like we may end up with 99% of cases where we're not gonna need initialValue. Introducing an "Onyx hook" will force everyone to provide that (I think). And to provide it in cases where they don't actually need it at all because it's logically impossible for the data not to have been loaded in the cache already.

Thanks!

ospfranco · 2023-09-11T08:00:15Z

pre-loading/pre-fetching is a solution, but it is not always applicable or practical.
For example, action reactions are not really linked to anything and it is not possible to know before hand which ones can be pre-loaded.

The reactions are something we could prefetch. They are linked to the actions via reportActionID, the actions are linked to the report via reportID, the reports are shown in the sidebar (and sometimes other places). We can prefetch the needed data on layout, scroll, hover, etc.

We could also layer something like this over your proposed changes later to improve the UX so I'd love to hear your thoughts on this idea either way.

Which data are we trying to load specifically that is causing a delay? Is it all stuff we need? Why is it slow? Can we do anything to load it faster?

Ah sorry, I thought reactions where only based on the actionID.

The point I'm trying to make is rather: before the user clicks on the report you don't really know what to pre-fetch. In order to make it really effective we would have to load the entire DB into memory.

Once the report is opened we basically load everything, it's done via different components ofc, but it ends up being the entire sub-tree of data linked to the clicked Report. Report, ReportMetadata, Actions, Reactions to those Actions, IOUReports, etc. Each one causes a delay at different components and times. And yes, it is all needed, we got rid of a couple of unneeded connections on the corresponding Expensify PR but nothing major. As far as I can see there is nothing to be done to load it faster.

Other entities might require loading the entire db into memory to be really effective.

Which entity? I feel like we are talking about some hypothetical mega component that for some reason needs the entire database in memory and actually can't think of what that would be.

Right now the ReportScreen is the biggest one, but as far as I'm told, into only grew in complexity somewhat recently. However, as time passes it is the natural progression more and more components will get connected to more entities.

The problem is not only one single component, but the entire sub-tree that gets rendered. It's on a case to case basis that sub-trees get expensive to render or cause a lot of re-renders. It's not about one entity, but rather how onyx holds rendering/pushes states and the interaction between components and React trying to flush/render components.

In any case, this is rather a techinique for further optimization and the current implemented solution allows for a more generic solution, which is allowing the components to render while state is being fetched.

Overall, this looks like a good idea that we probably need. I'm just trying to understand if we need it right now. It being a "generic solution" is part of my concern. Generic solutions sometimes add complexity or cause regressions for not much obvious benefit.

As stated, we could not apply pre-fetching to the ReportScreen and it's entire sub-tree of components that mounted and caused delays in rendering. The only solution we found was to allow the components to eagerly render. If we do not merge this change, we only apply superficial optimizations but the entire sub-tree will block as onyx pushes updates to the components.

It does also feel like we may end up with 99% of cases where we're not gonna need initialValue. Introducing an "Onyx hook" will force everyone to provide that (I think). And to provide it in cases where they don't actually need it at all because it's logically impossible for the data not to have been loaded in the cache already.

Unless you load the entire DB into cache, I believe it is impossible to predict a cache miss. As initialValue is also optional, it doesn't really affect how the current app is developed, in the future a hook can be introduced, but the current performance really makes the app unusable and we were tasked to find a solution for now :)

Let me know if you still have questions, or maybe we can jump into a call to have a faster feedback loop, we can even play around with your ideas if that is what you need, and hopefully the ideas will become clearer.

marcaaron

Couple small things / questions. I am more ok with the idea as a whole now after reviewing more thoroughly. Though, still kind of wonder about the behavior of initialValue and if it should maybe replace defaultValue.

lib/Onyx.js

lib/withOnyx.js

marcaaron · 2023-09-12T00:04:03Z

lib/withOnyx.js

+                        const key = Str.result(mapping.key, props);
+                        let value = Onyx.tryGetCachedValue(key, mapping);
+                        if (!value && mapping.initialValue) {
+                            value = mapping.initialValue;


FWIW if we end up implementing a hook for Onyx then we don't really need defaultProps at all as the values are not props anymore. The interaction becomes simpler because you can:

Stop using withOnyx()

Delay the rendering (or not) at your discretion

No more defaultProps

// I am not a prop. Find some other way to validate my type 🙃 const {value: allReports} = useOnyx({key: 'reports_', initialValue: {}}); if (empty(allReports)) { return null; }

oh yea, also, the TS migration appears to have proposed we do away with defaultProps entirely

marcaaron · 2023-09-12T00:24:49Z

lib/withOnyx.js

-                // This handles case when setState was called before the setWithOnyxState.
-                // For example, when an Onyx property was updated by keyChanged before the call of the setWithOnyxState.
-                this.setState((prevState) => {
-                    const remainingTempState = _.omit(this.tempState, _.keys(prevState));


Are we reintroducing a bug with this change? Seems like the previous author discovered some case where an existing state value could beat the value in tempState.

I don't quite understand how is this change correct, what if a new value is set for a key in the prevState? Then it will be ommitted?

I think the idea is that there can be a race between a new value calling setState() on withOnyx() and the time it takes to get this initial value from storage.

And if that happens then we should use the most recent value (the one that was set and not the stale value that we read).

Tbh - not in love with how this was implemented. I would prefer find some way to mark the data as stale if something changed it's value, but I guess we decided the correct place to do that would be in the setState() callback.

Ah, now I got it. Well, this is no longer correct if initialValue is passed, it will never update because the component starts with a value on that key.

I need to think about how to solve this.

lib/withOnyx.js

marcaaron · 2023-09-12T00:42:23Z

lib/withOnyx.js

+                        const key = Str.result(mapping.key, props);
+                        let value = Onyx.tryGetCachedValue(key, mapping);
+                        if (!value && mapping.initialValue) {
+                            value = mapping.initialValue;


Without the hook though - to me, this begs the question of whether the initialValue should later be restored if the value becomes undefined in the future. It's an "initial value", but easy to confuse it as a "default value".

Should I still provide a default via props or default props? In this case, yes. It is only used for setting the initial value.

README.md

ospfranco · 2023-09-13T10:30:52Z

@marcaaron I updated the merging algorithm between prevState and the new state on setWithOnyxState unfortunately it is far more complex. I also tried to update the comment to explain the situation. Could you review it one more time and carefully to make sure I did not miss anything?

tgolen · 2023-09-13T16:11:36Z

lib/withOnyx.js

@@ -19,7 +20,7 @@ function getDisplayName(component) {
    return component.displayName || component.name || 'Component';
 }

-export default function (mapOnyxToState) {
+export default function (mapOnyxToState, options = {}) {


Until there are other options, a simple param is fine.

Suggested change

export default function (mapOnyxToState, options = {}) {

export default function (mapOnyxToState, shouldDelayUpdates = false) {

I strongly argue against it, boolean params are confusing because they are not labeled. An options object future proofs while keeping things explicit.

While I don't disagree with the points of your logic, I disagree with the results of that logic. If that logic was followed, then no method anywhere would ever have single params and they would all have one big arguments object.

What about moving it to mapOnyxToState?

Robert C. Martin says it himself:

https://www.informit.com/articles/article.aspx?p=1392524

But Szymon didn't want the key inside mapOnyxToState because that would change how the necessary keys for rendering are discovered. I will change it as a single boolean for now.

Uncle Bob rules only apply if it's Java code

Why would it? it's just code and boolean functions. They get messy over time, there is plenty of arguments against them and I have also lived through long-lived codebases where they make everything hard to understand.

https://medium.com/@amlcurran/clean-code-the-curse-of-a-boolean-parameter-c237a830b7a3

https://understandlegacycode.com/blog/what-is-wrong-with-boolean-parameters/

In any case, not going to push this on you, just sharing my experience.

tgolen

Can you please add unit tests to ensure that initialValue and shouldDelayUpdates work as expected?

…ag for shouldDelayUpdates

tgolen

Thanks for making the change to the boolean param and for the tests.

tests/unit/withOnyxTest.js

tgolen · 2023-09-14T15:50:02Z

API.md

@@ -131,6 +131,7 @@ Subscribes a react component's state directly to a store key
 | [mapping.initWithStoredValues] | <code>Boolean</code> | If set to false, then no data will be prefilled into the  component |
 | [mapping.waitForCollectionCallback] | <code>Boolean</code> | If set to true, it will return the entire collection to the callback as a single object |
 | [mapping.selector] | <code>String</code> \| <code>function</code> | THIS PARAM IS ONLY USED WITH withOnyx(). If included, this will be used to subscribe to a subset of an Onyx key's data.       If the selector is a string, the selector is passed to lodashGet on the sourceData. If the selector is a function, the sourceData and withOnyx state are       passed to the selector and should return the simplified data. Using this setting on `withOnyx` can have very positive performance benefits because the component       will only re-render when the subset of data changes. Otherwise, any change of data on any property would normally cause the component to re-render (and that can       be expensive from a performance standpoint). |
+| [mapping.initialValue] | <code>Any</code> | THIS PARAM IS ONLY USED WITH withOnyx(). If included, this will be passed as the initial hydrated value for the mapping. It will allow the component to render eagerly while data is being fetched from the DB. Note that it will not cause the component to have the `loading` prop set to true. |


Is it expected that only initialValue is added to this API doc? It seems like shouldDelayUpdates and markReadyForHydration should show up somewhere here. Maybe try re-running the generator?

Ah, this document is generated? I modified manually

I re-ran the generator. However shouldDelayUpdates and markReadyForHidration are part of the HOC and not of Onyx, so not sure if I should put them here. That's why I also updated the README

Ah, OK... I guess that's fine if the generator didn't pick them up. I didn't think about it only being a part of the HOC. I think the stuff you added in the README is good enough for the HOC.

tgolen · 2023-09-14T15:51:11Z

API.md

@@ -131,6 +131,7 @@ Subscribes a react component's state directly to a store key
 | [mapping.initWithStoredValues] | <code>Boolean</code> | If set to false, then no data will be prefilled into the  component |
 | [mapping.waitForCollectionCallback] | <code>Boolean</code> | If set to true, it will return the entire collection to the callback as a single object |
 | [mapping.selector] | <code>String</code> \| <code>function</code> | THIS PARAM IS ONLY USED WITH withOnyx(). If included, this will be used to subscribe to a subset of an Onyx key's data.       If the selector is a string, the selector is passed to lodashGet on the sourceData. If the selector is a function, the sourceData and withOnyx state are       passed to the selector and should return the simplified data. Using this setting on `withOnyx` can have very positive performance benefits because the component       will only re-render when the subset of data changes. Otherwise, any change of data on any property would normally cause the component to re-render (and that can       be expensive from a performance standpoint). |
+| [mapping.initialValue] | <code>Any</code> | THIS PARAM IS ONLY USED WITH withOnyx(). If included, this will be passed as the initial hydrated value for the mapping. It will allow the component to render eagerly while data is being fetched from the DB. Note that it will not cause the component to have the `loading` prop set to true. |


the initial hydrated value for the mapping

What exactly does "hydrated" mean? 😅

Is there a better term we can use for this that has the same meaning? I can only think of someone drinking a lot of water.

Standard React lingo. But open to suggestions.

Heh, yeah, but what does it mean to you? Like, let's say you were explaining this to a 7-year-old.

I was talking with another co-worker about this and ChatGPT for the win!

So, I think it would be helpful to change this to say:

If included, this will be passed to the component so that something can be rendered while data is being fetched from the DB. Note that it will not cause the component to have the loading prop set to true.

Co-authored-by: Tim Golen <[email protected]>

# Conflicts: # API.md # lib/Onyx.js

ospfranco · 2023-09-16T07:57:03Z

PR is updated with latest comments

tgolen

Sorry, this is so close! A few conflicts snuck in from the last PR I merged.

# Conflicts: # lib/Onyx.js # lib/storage/providers/IDBKeyVal.js # lib/utils.js

ospfranco · 2023-09-18T06:56:06Z

Updated

marcaaron

One small comment about the mapping.config variable.

marcaaron · 2023-09-18T08:56:34Z

lib/withOnyx.js

+                        if (
+                            (value !== undefined
+                                && !Onyx.hasPendingMergeForKey(key))
+                            || mapping.allowStaleData


How does allowStaleData work? I don't see where that will be passed or any documentation added on it.

Looks, like this is the only place where it is used. Seems like it allows the component to be feed undefined data or stale data when there is pending merges. There is no documentation since it is not part of onyx.js which is the only file the generator looks at. That's why I manually added a section to the README.

marcaaron · 2023-09-18T09:06:32Z

lib/withOnyx.js

+                const stateUpdate = {...this.tempState};
+                delete this.tempState;
+
+                // Full of hacky workarounds to prevent the race condition described above.


NAB / FWIW, I am looking at this code and it looks reasonable since the edge case is well described and this clearly shows the few cases we will run into. Maybe I am a hacker though 😄 Nice work either way.

marcaaron · 2023-09-18T09:08:57Z

Looks like a test is failing as well.

ospfranco · 2023-09-18T09:32:58Z

Looks like a missed a change when I did the merge. Tests should pass now.

marcaaron · 2023-09-19T00:33:48Z

@yuwenmemon did you want to give this any final review?

ospfranco requested a review from a team as a code owner September 5, 2023 12:36

melvin-bot bot requested review from yuwenmemon and removed request for a team September 5, 2023 12:36

ospfranco mentioned this pull request Sep 5, 2023

Use initialValue in withOnyx and other optimizations for ReportScreen Expensify/App#26772

Merged

59 tasks

ospfranco changed the title ~~Add initialValue, delayUpdates, batching and collection mapping improvements~~ Add initialValue, delayUpdates, collection mapping improvements Sep 6, 2023

ospfranco mentioned this pull request Sep 6, 2023

Use a map for Onyx collection keys #330

Merged

ospfranco changed the title ~~Add initialValue, delayUpdates, collection mapping improvements~~ Add initialValue, delayUpdates to allow components to eagerly render. Sep 6, 2023

yuwenmemon requested changes Sep 6, 2023

View reviewed changes

Add initialValue, delayRendering and setStateProxy

56e6f84

Linting on utils

tgolen requested changes Sep 7, 2023

View reviewed changes

yuwenmemon requested changes Sep 7, 2023

View reviewed changes

ospfranco added 3 commits September 8, 2023 17:01

Move fastMerge to utils, add documenation for initialValue and delayU…

d5de1d4

…pdates

Linting, revert of multi line breakdowns

72d4a17

Fix utils comparisson

4396283

ospfranco requested review from marcaaron, yuwenmemon and tgolen September 8, 2023 15:17

chrispader mentioned this pull request Sep 11, 2023

Fix: nullish keys not removed/merged in IDB-Keyval (web) #333

Merged

marcaaron previously requested changes Sep 12, 2023

View reviewed changes

mountiny assigned ospfranco Sep 12, 2023

Minor fixes with new merging algo

8d5e11a

tgolen requested changes Sep 13, 2023

View reviewed changes

ospfranco added 2 commits September 14, 2023 12:30

Add tests for initial value and shouldDelayUpdates

d17c74f

Remove options object from withOnyx, replace with a single boolean fl…

1b92d73

…ag for shouldDelayUpdates

tgolen requested changes Sep 14, 2023

View reviewed changes

tests/unit/withOnyxTest.js Outdated Show resolved Hide resolved

tests/unit/withOnyxTest.js Outdated Show resolved Hide resolved

tests/unit/withOnyxTest.js Outdated Show resolved Hide resolved

tests/unit/withOnyxTest.js Outdated Show resolved Hide resolved

tgolen requested changes Sep 14, 2023

View reviewed changes

Update tests/unit/withOnyxTest.js

9fcd0e5

Co-authored-by: Tim Golen <[email protected]>

ospfranco requested a review from tgolen September 15, 2023 08:48

Szymon20000 mentioned this pull request Sep 15, 2023

Batch initial hydration. #345

Closed

ospfranco added 2 commits September 16, 2023 09:51

Update PR

eb78258

Merge branch 'main' into osp/report-screen-optimizations

7499568

# Conflicts: # API.md # lib/Onyx.js

tgolen previously approved these changes Sep 17, 2023

View reviewed changes

Merge branch 'main' into osp/report-screen-optimizations

5e3da59

# Conflicts: # lib/Onyx.js # lib/storage/providers/IDBKeyVal.js # lib/utils.js

ospfranco dismissed tgolen’s stale review via 5e3da59 September 18, 2023 06:55

tgolen previously approved these changes Sep 18, 2023

View reviewed changes

marcaaron requested changes Sep 18, 2023

View reviewed changes

Fix fast merge function

d689261

ospfranco dismissed tgolen’s stale review via d689261 September 18, 2023 09:32

marcaaron approved these changes Sep 19, 2023

View reviewed changes

yuwenmemon approved these changes Sep 19, 2023

View reviewed changes

yuwenmemon merged commit 6131a2b into Expensify:main Sep 19, 2023
3 checks passed

ospfranco deleted the osp/report-screen-optimizations branch September 19, 2023 05:57

tgolen mentioned this pull request Oct 5, 2023

Fix mSafari focus Expensify/App#23922

Closed

58 tasks

		const getSubsetOfData = (sourceData, selector, withOnyxInstanceState) =>
		selector(sourceData, withOnyxInstanceState);

		@@ -0,0 +1,3 @@
		export function areObjectsEmpty(a, b) {
		return typeof data === 'object' && typeof previousData === 'object' && Object.values(a).length === 0 && Object.values(b).length === 0

		@@ -0,0 +1,12 @@
		import * as _ from 'underscore';

		function areObjectsEmpty(a, b) {

	const propsToPass = _.omit(this.props, value => _.isNull(value));
	const propsToPass = _.omit(this.props, _.isNullvalue);

	export default function (mapOnyxToState, options = {}) {
	export default function (mapOnyxToState, shouldDelayUpdates = false) {

Add initialValue, delayUpdates to allow components to eagerly render. #329

Add initialValue, delayUpdates to allow components to eagerly render. #329

Conversation

ospfranco commented Sep 5, 2023 • edited by mountiny Loading

Details

Related Issues

Automated Tests

Linked PRs

github-actions bot commented Sep 5, 2023 • edited Loading

ospfranco commented Sep 6, 2023

yuwenmemon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Sep 6, 2023

ospfranco commented Sep 7, 2023

tgolen commented Sep 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Sep 7, 2023

yuwenmemon left a comment

Choose a reason for hiding this comment

ospfranco commented Sep 8, 2023

ospfranco commented Sep 8, 2023

marcaaron commented Sep 8, 2023

ospfranco commented Sep 11, 2023 • edited Loading

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ospfranco commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ospfranco commented Sep 16, 2023

tgolen left a comment

Choose a reason for hiding this comment

ospfranco commented Sep 18, 2023

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Sep 18, 2023

ospfranco commented Sep 5, 2023 •

edited by mountiny

Loading

github-actions bot commented Sep 5, 2023 •

edited

Loading

ospfranco commented Sep 11, 2023 •

edited

Loading