Expensify
[Performance] useOnyx doesn't return new data if the selector changes but the data in the store is the same
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
What performance issue do we need to solve?
Inefficient React component rendering when onyx collection data is updated, although we're only accessing unmodified entries of the onyx collection.
Example case: https://github.com/Expensify/App/pull/49919#pullrequestreview-2346813622
What is the impact of this on end-users?
App performance can degrade with the inefficient rendering
List any benchmarks that show the severity of the issue
N/A
Proposed solution (if any)
Mimic dependency array from some React hooks, like useEffect, to force useOnyx to fetch data if any dependency changes.
Something like useOnyx(key, options, [dependencyA]).
List any benchmarks after implementing the changes to show impacts of the proposed solution (if any)
N/A
Platforms:
Which of our officially supported platforms is this issue occurring on?
- [x] Android: Native
- [x] Android: mWeb Chrome
- [x] iOS: Native
- [x] iOS: mWeb Safari
- [x] MacOS: Chrome / Safari
- [x] MacOS: Desktop
Version Number: v9.0.62-4 Reproducible in staging?: yes Reproducible in production?: yes Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: https://github.com/Expensify/App/pull/49919#pullrequestreview-2346813622 Expensify/Expensify Issue URL: Issue reported by: Slack conversation:
Issue Owner
Current Issue Owner: @pac-guerreiro
Auto-assigning issues to engineers is no longer supported. If you think this issue should receive engineering attention, please raise it in #whatsnext.
![melvin-bot[bot] avatar](https://avatars.githubusercontent.com/in/179547?v=4 "Published by a pub.dev verified publisher"){kind=small}
@mountiny can you make this issue internal? @fabioh8010 and I are working on it
Last friday I didn't work on this but tomorrow I'll open a draft PR with a possible solution
I opened a draft PR with a possible solution for this issue - https://github.com/Expensify/react-native-onyx/pull/599
If you all agree, I can also create an eslint rule to enforce the usage of the dependency array when developers pass variables in useOnyx selector. To achieve it, I'll need to adapt this and create an additional PR to add this new rule to https://github.com/Expensify/eslint-config-expensify
I'm waiting on @fabioh8010 feedback on this since he has an alternate solution
Today @fabioh8010 and I discussed this issue internally. There are some ideas on the table on how to tackle this issue, so we'll continue discussing them tomorrow!
Today @fabioh8010 continued our discussion, we are still trying to find a proper solution for this issue.
Today I tried more possible solutions and discussed them with @fabioh8010. I've been having problems with infinite loops on getSnapshot. On Monday I'm going to discuss an alternative idea of @fabioh8010 for this performance issue.
@fabioh8010, @mountiny, @pac-guerreiro Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
Yesterday I was able to provide a possible solution to the problem and created a draft PR - https://github.com/Expensify/react-native-onyx/pull/601
@fabioh8010 provided a solution of his, which I didn't have the time to check yesterday but today I'll do it!
Today I reviewed @fabioh8010 solution and we discussed it. We need to confirm which one is the best and how we should proceed next
Taking this issue over Pedro since he's going to be OOO until December 12th
@fabioh8010, @mountiny, @pac-guerreiro Eep! 4 days overdue now. Issues have feelings too...
Update: We agreed to test my solution against E/App and if everything is ok, we will proceed with it.
@fabioh8010, @mountiny, @pac-guerreiro 6 days overdue. This is scarier than being forced to listen to Vogon poetry!
@fabioh8010, @mountiny, @pac-guerreiro Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!
@mountiny Could you take a quick 👀 in this PR? Onyx main is currently broken and we need that fix before proceeding with the Bump PR. Thanks!
@fabioh8010, @mountiny, @pac-guerreiro 12 days overdue. Walking. Toward. The. Light...
Reviewing label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.76-12 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
- https://github.com/Expensify/App/pull/53890
If no regressions arise, payment will be issued on 2024-12-25. :confetti_ball:
For reference, here are some details about the assignees on this issue:
- @fabioh8010 does not require payment (Contractor)
- @pac-guerreiro does not require payment (Contractor)