Expensify / react-native-onyx

Persistent, offline-first state management solution for React Native. Easy to use with minimal config and boilerplate.
MIT License
159 stars 72 forks source link

Onyx connection can be called twice for the same data #569

Closed hannojg closed 1 month ago

hannojg commented 3 months ago

There exists a "race condition" where an Onyx connection callback will be called twice for the same data. Potentially this can lead to unnecessary rerenders in the react applications.

The race condition happens when creating a connection and shortly after setting the data:

Screenshot 2024-07-18 at 15 12 29

It once gets called here in .set:

https://github.com/Expensify/react-native-onyx/blob/c53826bbfe31e423fad0c33b0cb9ab097b88f128/lib/Onyx.ts#L274-L275

and then here once the connection promise resolved:

https://github.com/Expensify/react-native-onyx/blob/c53826bbfe31e423fad0c33b0cb9ab097b88f128/lib/Onyx.ts#L175

hannojg commented 3 months ago

I think I could build a failing unit test and fix the issue (cc @mountiny @marcaaron )

mountiny commented 3 months ago

@hannojg yes please, feel free to investigate a fix for this

hannojg commented 2 months ago

PR is up here:

mountiny commented 2 months ago

@hannojg are you able to create the onyx bump now with the checklist filled in? thanks!

hannojg commented 2 months ago

Hey, yes I could, but I think you mentioned me in some other issue and someone else is doing the bump, no?

mountiny commented 2 months ago

Yeah that was after i commented here, i think they can handle that for you

hannojg commented 2 months ago

My first PR caused a few test to fail in the newdot code. I addressed the problems in these PRs (would appreciate a review):

mountiny commented 2 months ago

@hannojg I believe these changes are now live in the App, thank you! Do you reckon we can close this issue out?

hannojg commented 1 month ago

Yes, thats fixed now!