search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.48k stars 2.84k forks source link

[HOLD for payment 2024-04-25] [Tracking] [Performance] Enable the new Fabric architecture #8503

Closed mvtglobally closed 5 months ago

mvtglobally commented 2 years ago

GitHub Project: https://github.com/orgs/Expensify/projects/35

Main New Arch PR: https://github.com/Expensify/App/pull/13767

Context

New render engine: https://reactnative.dev/architecture/fabric-renderer Summary of findings 1) There are 4 main components of RN: React, JavaScript, a series of elements collectively known as the “bridge”, and the native elements/modules. Each of these components are getting complimentary upgrades. 2) The notable React upgrades discussed are concurrent rendering, synchronous event callbacks, and priority rendering queues. Together those improvements basically allow for the UI thread to perform heavy rendering work in the background without compromising quick response times to user events such as scrolling and navigation gestures. 3) In the existing RN architecture, the JS and native sides are essentially unaware of each other. All communications are serialized JSON messages sent asynchronously over the bridge. 4) When JavaScript communicates with a web browser, the browser objects hold direct references to native input elements which are controlled internally by the browser. So an HTML checkbox in a browser actually internally holds a reference to a native macOS or Windows checkbox that’s managed by the browser. Similarly, JSI (JavaScript Interface) is a new piece of RN that enables direct communication between JS and the native side, because JSI internally manages direct references to C++ host objects (similar to the HTMLInput element on a browser). This means that messages sent between JS and native modules no longer need to be JSON-serialized or sent over the single bottleneck communication channel. It also means messages can be sent either synchronously or asynchronously. 6) At a high level, the traditional RN bridge manages two tasks: computing layout via a component tree called the “shadow tree” (that shadows the React tree), and managing native modules such as the camera. In the new architecture, these are broken into two distinct pieces which facebook is calling Fabric and TurboModules, respectively. 6) Fabric, the new UI manager, leverages JSI to create the shadow tree in C++ directly, cutting out the middle man and dramatically improving performance. It also exposes UI operations on both sides, so the native side and JS side can both directly manipulate the shadow tree without having to send serialized messages back and forth across the bridge. 7) In the existing native module system, all the native modules need to be loaded when the app launches so that they’re ready and waiting for commands to be sent over the bridge. Turbo Modules have direct references to native modules via JSI, which gives two main benefits. First, native modules can be lazy-loaded (only when really needed), which should dramatically improve app startup time, especially on apps that use lots of different native modules. Second, once loaded the modules will be more performant because communications no longer need to be JSON-serialized and sent across the bridge. 8) Lastly, there is a tool I don’t fully understand called CodeGen. It leverages TypeScript to generate the interface files needed by Fabric and TurboModules. This compile-time type safety means that the code counterparts from both the native and JS worlds can trust eachother without any runtime checks, which means smaller bundle sizes and faster execution.

Requirements

Migrate off setNativeProps

Migrate off findNodeHandle

Bidirectional scrolling

Dependency upgrades

Known bugs

Expensify/Expensify Issue URL: Issue reported by: @roryabraham Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1648743262264529

melvin-bot[bot] commented 2 years ago

Triggered auto assignment to @stitesExpensify (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

roryabraham commented 2 years ago

This can definitely be external. I went ahead and flagged it as a potential issue for the Margelo team to work on: https://expensify.slack.com/archives/C035J5C9FAP/p1649217447621559

mrousavy commented 2 years ago

Awesome, Margelo would love to work on this! We noticed that you use a lot of third party React Native libraries, so it'll be a bit difficult to get those migrated as well. Especially problematic is Reanimated, but the SWM team is working on bringing that to Fabric.

Let's coordinate internally which issue we want to prioritize :)

stitesExpensify commented 2 years ago

Making this a weekly while we prioritize

stitesExpensify commented 2 years ago

Moving to monthly

stitesExpensify commented 2 years ago

@mrousavy just for clarity on this issue, where is it currently prioritized?

mrousavy commented 2 years ago

I believe currently we are not working on this issue, maybe let's discuss this in the Slack channel? We are currently working a lot with Coinbase and have very limited availability, but we expect to have some in two weeks!

kidroca commented 2 years ago

I've stumbled on this ticket thanks to experimenting with Fabric and the new RN Architecture. It turns out migrating would resolve one top paying issue here: https://github.com/Expensify/App/issues/8349#issuecomment-1151172897

I think we can

I'm posting a comment to keep in touch I'll be happy to work or take part in landing the new architecture in App

stitesExpensify commented 2 years ago

Thanks for volunteering @kidroca! There is still a lot of internal conversation happening around this, so I'll post an update when I have one 😄

roryabraham commented 2 years ago

Relevant discussion: https://expensify.slack.com/archives/C035J5C9FAP/p1655407547697249

stitesExpensify commented 2 years ago

No updateyet

stitesExpensify commented 2 years ago

@roryabraham are we still planning on doing this?

roryabraham commented 2 years ago

yes! We are now on our forked equivalent of the upstream RN 0.69.3. As soon as we can turn on Fabric without introducing regressions, we absolutely should!

The flag is there, so we can enable Fabric. We just need to make sure all our dependencies are compatible and test rigorously to make sure we don't break things.

rushatgabhane commented 2 years ago

If you didn't already know, setNativeProps will not be supported in the post-Fabric world (docs).

Here’s a list of NewDot components already using it - (github)

roryabraham commented 2 years ago

Oh, nice! We should get ahead of that and migrate away from setNativeProps usages ASAP.

stitesExpensify commented 2 years ago

Agreed. @rushatgabhane i'll create an issue, do you want me to just assign you, or would you prefer a contributor take it on?

roryabraham commented 2 years ago

Another one:

Warning: ReactDOM.render is no longer supported in React 18. Use createRoot instead. Until you switch to the new API, your app will behave as if it's running React 17. Learn more: https://reactjs.org/link/switch-to-createroot

parasharrajat commented 2 years ago

Ok. So it is going to be some cleanup work. Please tag me on the issue, I want to track its progress.

rushatgabhane commented 2 years ago

do you want me to just assign you

@stitesExpensify yess, that'd be great! It'll require a lot of regression testing but it should be fun.

roryabraham commented 2 years ago

Will also need to redo https://github.com/Expensify/App/pull/10334 before we can turn on Fabric (it caused a crash as was reverted)

rushatgabhane commented 2 years ago

I'm stuck here 😂. I'm trying to move setNativeProps to state.

Which prop of View do I need to use to set method and action?

https://github.com/Expensify/App/blob/d7a7334abbdd699510bc98f566c5f02b12ec46b8/src/components/SignInPageForm/index.js#L10-L14

cc: @parasharrajat any insights?

parasharrajat commented 2 years ago

This is a good topic for slack. You will have to use the form element instead of View or Text in the web-specific file.

OR

Just set the attribute via setAttribute.

parasharrajat commented 2 years ago

I guess setAttribute is better.

rushatgabhane commented 2 years ago

Just set the attribute via setAttribute

Yes, that works. Thank you so much! @parasharrajat

rushatgabhane commented 2 years ago

@parasharrajat are you interested in reviewing PR #10934?

If yes, @stitesExpensify could you please assign him to the PR?

parasharrajat commented 2 years ago

Its a big one but I am up for it.

trjExpensify commented 2 years ago

@stitesExpensify is out of the office this week, FYI. Feel free to pull the trigger on puller bear for the internal assignment portion.

rushatgabhane commented 2 years ago

Thanks for letting us know.

rushatgabhane commented 2 years ago

As per @luacmartins' suggestion - I split the migration of setNativeProps PR https://github.com/Expensify/App/pull/10934 into the following PRs to make reviews and testing easier.

Let me know if I missed something.

cc: @parasharrajat I wrote all the tests, but I'll add the screenshots by next week (low bandwidth). I hope that's ok.

parasharrajat commented 2 years ago

I am confused about this issue. the Expected result can't be achieved in one PR.

We should break this into multiple issues.

  1. Version upgrade to 0.68 is already done.
  2. Enable fabric architecture is a different issue.
  3. Rushat is working on a few things which do not solve this (but are needed). Create a separate issue for this. (title may be Migrate from SetNativeProps to state).
trjExpensify commented 2 years ago

Yeah, it might be helpful to start treating this issue more like a main/tracking issue. The OP containing a checklist of the prerequisite issues we need to complete to be in a position to actually enable Fabric. That said, outside of deprecating SetNativeProps, do we have any other prerequisites that need to be completed at this point?

parasharrajat commented 2 years ago

Currently, I only know these.

rushatgabhane commented 2 years ago

do we have any other prerequisites that need to be completed at this point?

Yes!

https://expensify.slack.com/archives/C01GTK53T8Q/p1662132372464699?thread_ts=1662114154.919809&cid=C01GTK53T8Q

https://expensify.slack.com/archives/C01GTK53T8Q/p1662162747229869?thread_ts=1662114154.919809&cid=C01GTK53T8Q

luacmartins commented 2 years ago

I agree with @trjExpensify that we should use this issue as a tracking issue. I went ahead and created separate issues for each setNativeProps migration and assigned them to you @rushatgabhane. I also updated the OP to track them and the existing PRs to link to the issues as well.

@rushatgabhane are there any other existing issues or PRs for the other requirements in these lists?

https://expensify.slack.com/archives/C01GTK53T8Q/p1662132372464699?thread_ts=1662114154.919809&cid=C01GTK53T8Q

https://expensify.slack.com/archives/C01GTK53T8Q/p1662162747229869?thread_ts=1662114154.919809&cid=C01GTK53T8Q

rushatgabhane commented 2 years ago

Thanks for creating the issues!

are there any other existing issues or PRs for the other requirements in these lists?

@luacmartins I don't believe so.

marcaaron commented 2 years ago

Quick question here and sorry if it's already been mentioned...

But why is setNativeProps() not supported in fabric? And rather than refactor everything to not use it - did we explore just stubbing it in somehow so we can unlock using fabric now and then change all the components later?

marcaaron commented 2 years ago

Seems like there is some related stuff here and here. Does fabric pretty much just solve all the performance issues that led people to use uncontrolled components in the first place?

luacmartins commented 2 years ago

But why is setNativeProps() not supported in fabric?

Not too sure. The docs just mention that it's not supported 😭 They also mention that Fabric Native Components achieve similar results to setNativeProps.

did we explore just stubbing it in somehow so we can unlock using fabric now and then change all the components later?

I didn't explore any alternative solutions yet, but maybe @rushatgabhane did? I remember that when implementing Form setNativeProps was the only solution that worked on native without using state, but I don't recall the specific reason.

marcaaron commented 2 years ago

It would be good to rule out any obvious alternatives, but nothing is really coming to mind. Not entirely sure how you'd implement a setNativeProps(). Maybe it is less work than I am imagining it will be.

Maybe the best thing is to just start with one migration and verify performance does not get worse. Then again now sure how easy that will be since we'd maybe need to refactor all components to even start using the new version 😅

rushatgabhane commented 2 years ago

Asked for alternatives in react working group. https://github.com/reactwg/react-native-new-architecture/discussions/77

And added a feedback for docs to mention why it was deprecated in the first place https://github.com/reactwg/react-native-new-architecture/discussions/8#discussioncomment-3670468

roryabraham commented 2 years ago

Going to co-assign here and help out, particularly when it comes to upgrading our fork as needed. Working on a PR to upgrade to 0.70.2 (based on 0.70.1 in the upstream)

trjExpensify commented 2 years ago

This is tied to WN, so I'm updating the priority label on this tracker to weekly for updates. As per this thread, adding a bullet to the OP for:

roryabraham commented 2 years ago

Added a new section for dependency upgrades we'll need. One that stands out is that we'll need to upgrade to Reanimated 3, which is still in beta but will probably enter LTS soon

Reanimated 3 will be the first version of the library that supports the new React Native architecture — Fabric

parasharrajat commented 2 years ago

I am also waiting on Reanimated for jest tests.

roryabraham commented 2 years ago

Updated this issue to include a requirement to address this warning:

Warning: ReactDOM.render is no longer supported in React 18. Use createRoot instead. Until you switch to the new API, your app will behave as if it's running React 17

Unless I'm mistaken, we can't get the benefits from Fabric unless we are actually using React 18

rushatgabhane commented 2 years ago

Warning: ReactDOM.render is no longer supported in React 18. Use createRoot instead

@roryabraham is anyone working on this? If no, then I could do the chore.

I'm assuming this a react-native-web only problem.

Using createRoot and hydrateRoot from react-dom/client should do the trick. (source: react-working-group)

The changes will be made in Expensify's fork - RN-web/render/index.js

kidroca commented 2 years ago

Updated this issue to include a requirement to address this warning:

Warning: ReactDOM.render is no longer supported in React 18. Use createRoot instead. Until you switch to the new API, your app will behave as if it's running React 17

Unless I'm mistaken, we can't get the benefits from Fabric unless we are actually using React 18

This is a react-dom (renderer) warning. Fabric is only for react-native (renderer) We should still address it, but it's not relevant to the Fabric architecture (React 18 is the core, but the renderer implementations like native and dom are independent)

parasharrajat commented 2 years ago

Looks like there is a PR already for it https://github.com/necolas/react-native-web/pull/2330. we can wait for the RN-web to release support for React 18.

rushatgabhane commented 2 years ago

there is a PR already for it

That's very convenient :)

roryabraham commented 2 years ago

Added https://github.com/Expensify/App/issues/11635 to the issue description