react-native-onyx icon indicating copy to clipboard operation
react-native-onyx copied to clipboard

Onyx connection can be called twice for the same data

Open hannojg opened this issue 1 year ago • 3 comments

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 avatar Jul 18 '24 13:07 hannojg

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

hannojg avatar Jul 18 '24 13:07 hannojg

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

mountiny avatar Jul 20 '24 00:07 mountiny

PR is up here:

  • https://github.com/Expensify/react-native-onyx/pull/570

hannojg avatar Jul 22 '24 11:07 hannojg

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

mountiny avatar Jul 25 '24 23:07 mountiny

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

hannojg avatar Jul 26 '24 09:07 hannojg

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

mountiny avatar Jul 26 '24 10:07 mountiny

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

  • https://github.com/Expensify/react-native-onyx/pull/573
  • https://github.com/Expensify/react-native-onyx/pull/574
  • https://github.com/Expensify/react-native-onyx/pull/575

hannojg avatar Jul 30 '24 20:07 hannojg

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

mountiny avatar Aug 14 '24 20:08 mountiny

Yes, thats fixed now!

hannojg avatar Aug 22 '24 14:08 hannojg