[Due for payment 2025-04-25] [CRITICAL] Add canBeMissing configuration option to useOnyx

[iwiznia]

Context https://expensify.slack.com/archives/C03TQ48KC/p1741208342513379

Problem:

The backend doesn't know all the data the frontend needs, the frontend doesn't know if the we actually loaded from the backend all the data it is expecting/using. This leads to hard to debug/reproduce bugs when frontend tries to access a piece of data it thinks should exist but wasn't really loaded.

Solution:

Add a way so that we can indicate in the useOnyx calls if the data is ok to be missing or if we were expecting it to be there for sure:

• We will add a new param to useOnyx called canBeMissing
• We will add an ESLint rule so that all calls to useOnyx need to pass it (this is to allow us to add this param little by little instead of having to audit all useOnyx calls at once)
• When the param is false and the useOnyx call returns no data, we will log an alert in production (which will create issues for us to investigate). In dev we will instead throw, to try to catch the problem before it hits production, when you are working on dev (we need to double check this, if it is throwing everywhere then let's just log instead, as we don't want to break everything)
• Once all places that call useOnyx are passing the param, we will remove the ESLint rule and make the param in useOnyx mandatory
• To handle the case where data is being loaded from the DB/cache, the alert won't be logged in the case status is loading and when status changes to loaded then it would log if data is null and canBeMissing was true.

[fabioh8010]

Hey, I'm Fábio - expert agency contributor - and I would like to work on this issue!

[fabioh8010]

As discussed in Slack I will start looking mid next week

[iwiznia]

We will add an ESLint rule so that all calls to useOnyx need to pass it (this is to allow us to add this param little by little instead of having to audit all useOnyx calls at once)

Thinking about this, I am not sure it will work since IIRC the ESLint rules in App run for all files and not only modified files?

[mallenexpensify]

Should this be on #quality as MEDIUM cuz of the below?

• MEDIUM - Debugging Tools to help make fixing existing bugs easier and getting fixes to customers faster and Architectural Improvements to the underlying framework and design of the app to improve performance, scalability, and maintainability

[iwiznia]

This is holding other issues marked as critical, so I assume should be critical too.
Anyway, @fabioh8010 said he was going to work on this mid this week, so hopefully we have a PR soon.

[fabioh8010]

We will add an ESLint rule so that all calls to useOnyx need to pass it (this is to allow us to add this param little by little instead of having to audit all useOnyx calls at once)

Thinking about this, I am not sure it will work since IIRC the ESLint rules in App run for all files and not only modified files?

@iwiznia We can use .eslintrc.changed.js to achieve this. It will only execute for the changed files of a PR.

---

When the param is false and the useOnyx call returns no data, we will log an alert in production (which will create issues for us to investigate). In dev we will instead throw, to try to catch the problem before it hits production, when you are working on dev (we need to double check this, if it is throwing everywhere then let's just log instead, as we don't want to break everything)

I think we shouldn't throw errors on dev, I believe it could lead to widespread crashes if for example we have an useOnyx with canBeMissing set to false that is inside a component or hook that is used throughout the codebase.

[fabioh8010]

Updates:

• Created first implementation of the feature and some unit tests. WIP Draft PR here.
• Will also proceed to start implementation of the ESLint rule, so I can test both together in advance.
• @iwiznia I will leave a question for you in the PR about a use case that I'm not sure.

[iwiznia]

@iwiznia We can use .eslintrc.changed.js to achieve this. It will only execute for the changed files of a PR.

Ah perfect!

I think we shouldn't throw errors on dev, I believe it could lead to widespread crashes if for example we have an useOnyx with canBeMissing set to false that is inside a component or hook that is used throughout the codebase.

Yeah, makes sense, what can we do instead? Maybe show a modal similar to the one we show when the backend does many writes? I want it to be something that's quite in your face.

[fabioh8010]

Yeah, makes sense, what can we do instead? Maybe show a modal similar to the one we show when the backend does many writes? I want it to be something that's quite in your face.

@iwiznia Could you show me where is this modal or how can I trigger this? Curious about this

[fabioh8010]

Updates:

• Addressed review comments and wait for final decision here before I proceed with tests, polishing and opening PR to review.
• Started working on ESLint rule,

[iwiznia]

@iwiznia Could you show me where is this modal or how can I trigger this? Curious about this

Oh, I was referring about this, but it seems it just uses an alert, since this is dev only I guess we can also use alert. I don't think you can test this, other than modifying that code and triggering it manually, since that data is never returned in the production/staging api, only in dev.

[fabioh8010]

Updates:

• Opened PR for ESLint rule here.
• Addressed comment in the PR about selector.
• About this, left a comment in the PR.

[fabioh8010]

Updates:

• ESLint PR was merged!
• Opened Onyx PR to review!

[fabioh8010]

Updates:

• Addressed comments in Onyx PR. Waiting for another round of reviews.

[iwiznia]

Great progress here, thanks so much @fabioh8010! Both PRs are merged, all it's missing is adding it to App, right?

[fabioh8010]

Yes! Created Draft PR here -> #59191
However I would like to do some more testing before opening.

[fabioh8010]

Basically I'm worried if we are going to have "false-positives" when signing out the user, as Onyx data is going to be cleared (and possibly returning undefined to some hooks) when doing so.

[melvin-bot]

@iwiznia, @fabioh8010 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[zirgulis]

PR for adding growl alerts instead of system alerts: #61056

[melvin-bot]

@iwiznia, @fabioh8010 Huh... This is 4 days overdue. Who can take care of this?

[melvin-bot]

Current assignee @mallenexpensify is eligible for the Bug assigner, not assigning anyone new.

[mallenexpensify]

@parasharrajat you'll be due $250 for the review of #61056 , correct?
Are you due $250 (had $350 here, was a typo) for #59191 too?

Anyone else due 💰 here?

[parasharrajat]

@mallenexpensify This issue was critical, so the pay would be 500 for this issue.
#61056 will be handled separately as a new change done later on #61076

[mallenexpensify]

@parasharrajat , this was updated to CRITICAL on March 18th, I think that was before I started updating CRITICAL issues to $500. (also.. it's not a set-in-stone rule that all CRITICAL will be $500, I do think that most/many should be, if they're External, to help get 👀 though). I want to avoid having to retroactively review many issues to potentially increase the amount.

Do you feel the amount of work, complexity or scope increased on the issue or PR? That could be reason for a potential price increase/

[parasharrajat]

I am fine with 250 as well for this issue instead of 500.

[mallenexpensify]

Contributor+: @parasharrajat due $250 via NewDot

Unsure if can close so I'll leave open til it goes overdue again

[mallenexpensify]

Closing

[ChavdaSachin]

Also @ChavdaSachin you'll be invited to #expensify-open-source pretty soon. I need to create an automated process for new invites and I haven't been able to prioritize it quite yet.

@mallenexpensify do you have any update on this?
I could help you voluntarily to automate the task if it is possible.

I am really stuck on an issue for about 15 days and it is really difficult for externals to work without being on slack.

Thank you.

[mallenexpensify]

@ChavdaSachin , the co-worker I'm working with on the automation returns to work on Monday, so hopefully it won't be too much longer

I am really stuck on an issue for about 15 days

Feel free to tag me in the issue to see if I can help.