Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
hannojg opened this issue Jul 18, 2024 · 9 comments
Closed

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

hannojg opened this issue Jul 18, 2024 · 9 comments
Assignees

Comments

@hannojg
Copy link
Contributor

hannojg commented Jul 18, 2024

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:

// This approach prioritizes fast UI changes without waiting for data to be stored in device storage.
const updatePromise = OnyxUtils.broadcastUpdate(key, valueWithoutNullValues, hasChanged);

and then here once the connection promise resolved:

OnyxUtils.get(mapping.key).then((val) => OnyxUtils.sendDataToConnection(mapping, val as OnyxValue<TKey>, mapping.key, true));

@hannojg
Copy link
Contributor Author

hannojg commented Jul 18, 2024

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

@mountiny
Copy link
Contributor

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

@hannojg
Copy link
Contributor Author

hannojg commented Jul 22, 2024

@mountiny
Copy link
Contributor

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

@hannojg
Copy link
Contributor Author

hannojg commented Jul 26, 2024

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

@mountiny
Copy link
Contributor

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

@hannojg
Copy link
Contributor Author

hannojg commented Jul 30, 2024

@mountiny
Copy link
Contributor

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

@hannojg
Copy link
Contributor Author

hannojg commented Aug 22, 2024

Yes, thats fixed now!

@hannojg hannojg closed this as completed Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants