[HOLD for payment 2024-07-22] LOW: Enable Concurrent React Features

[roryabraham]

Problem

There are a number of performance issues that we have long-term aspirations to solve via the React concurrency features introduced in React 18+. This was a big portion of the motivation to enable the new React Native architecture.

While we are still working on the New Arch on iOS and Android, we already have the ability to enable React concurrent mode on web. However, during the RNW 0.19 upgrade, it was found that some aspects of our codebase, particularly withOnyx, are not compatible with concurrent mode, so we turned it off.

Solution

Let's investigate what it will take to re-enable React Concurrent Mode and start working on that in parallel to the New Arch upgrade, which is well over a year in the making and should land hopefully by the end of January.

Issue Owner

Current Issue Owner: @muttmuure

[melvin-bot]

Triggered auto assignment to @anmurali (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[roryabraham]

Reassigning to @muttmuure for tracking as part of the NewDot Performance project

[roryabraham]

Put out a call for volunteers: https://expensify.slack.com/archives/C05LX9D6E07/p1703292300758289

[roryabraham]

Still looking for a volunteer, but I assume the people I tagged in slack may be partially OOO. This will remain on HOLD for this week, so I'll bump next week if I haven't heard anything

[roryabraham]

On HOLD for New Arch

[roryabraham]

On HOLD for New Arch

[roryabraham]

Still on HOLD

[roryabraham]

Though, we could try to get a volunteer to start looking into enabling this on web to get a headstart?

[roryabraham]

Posted again in slack: https://expensify.slack.com/archives/C035J5C9FAP/p1706561258788279?thread_ts=1706196763.533119&cid=C035J5C9FAP

[Szymon20000]

Ready for assignment :)

[roryabraham]

For now, the only platform that's ready for this is web, so you can focus your efforts there to start (while we're working on New Arch)

[Szymon20000]

Hello @roryabraham !
I spent around 3h today on investigating it and wanted to share my thoughts.
I would split the work here into 2 parts.
The first one is making sure onyx is ready for the concurrent rendering and the second one is that all of our components in the app don't have any side effects that could break concurrent rendering.

When it comes to onyx one I see 2 potential issues:

• The first one is that we have optimistic reads from cache in withOnyx component. Because concurrent rendering can (and most likely will) split the rendering into multiple parts it's possible that some of the components will read newer state than the others. We probably will need to use useSyncExternalStore here (Maybe by wrapping withOnyx in functional component). We already have allowStaleData option so maybe reading inconsistent state is fine in this case.

• Second one is of course sending updates to withOnyx components. What can happen now is that we will call setState on the first withOnyx listener but the rest will be batched later so again we ends up inconsistent state as some components already know the newer value. I remember that someone was already working on adding Jotai to onyx. Jotai already has support for react concurrent rendering and would solve that problem. What's the current state of it? If we don't plan to use Jotai in onyx we will need to use useSyncExternalStore hook.

For the second part (so making sure all components don't have any dangerous side effect) we can use StrictMode component from React (https://react.dev/reference/react/StrictMode#strictmode) and just traverse the whole app too everything works properly and we don't have any warning on the console. I already added that locally and the report screen stopped to work.

[roryabraham]

The first one is that we have optimistic reads from cache in withOnyx component ... We probably will need to use useSyncExternalStore here

Fortunately we have an issue to implement a useOnyx hook and it's happening! The current proposal is to use the useSynxExternalStore hook 👍🏼

I remember that someone was already working on adding Jotai to onyx ... What's the current state of it? If we don't plan to use Jotai in onyx we will need to use useSyncExternalStore hook.

This is the first I've heard of adding Jotai to Onyx, but as I just mentioned we'll be setting up useSyncExternalStore soon. Maybe we'll have to update withOnyx to internally use useOnyx?

For the second part (so making sure all components don't have any dangerous side effect) we can use StrictMode component from React (https://react.dev/reference/react/StrictMode#strictmode) and just traverse the whole app too everything works properly and we don't have any warning on the console

👍🏼 to enabling strict mode on dev. Maybe with a config-level flag to easily enable/disable it so we can turn it off and on easily.

I already added that locally and the report screen stopped to work

Great 🙃

So it sounds like this is the best thing to investigate next? Or do you think you'll be blocked on Onyx needing to use useSyncExternalStore?

[rayane-djouah]

No regression test is needed

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.6-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• [No QA] Update react-navigation StrictMode patch to work also for native apps #44726

If no regressions arise, payment will be issued on 2024-07-22. 🎊

For reference, here are some details about the assignees on this issue:

• @Kicu does not require payment (Contractor)
• @rayane-djouah requires payment (Needs manual offer from BZ)

[melvin-bot]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@rayane-djouah] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [@muttmuure] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[melvin-bot]

Payment Summary

Upwork Job

• Contributor: @Kicu is from an agency-contributor and not due payment
• ROLE: @rayane-djouah paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@muttmuure)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[muttmuure]

Created an issue here: https://www.upwork.com/jobs/~01cf7ff3f004364329

[muttmuure]

Upwork job got mangled

[muttmuure]

$250 for @rayane-djouah

[rayane-djouah]

@muttmuure - I accepted the invite

[muttmuure]

Offer sent

[rayane-djouah]

Offer accepted

[rayane-djouah]

cc @muttmuure

[muttmuure]

Paid