Support for New Architecture - iOS & Android (Fabric) #931

markrickert · 2023-10-10T00:22:25Z

Description

This PR builds on @j-piasecki's work in #550 to enable Fabric/New Architecture. That PR is fairly old and was created with [email protected] (lots of performance issues) and [email protected].

Here's what I did to bring things back up to date so that people can start testing this with the latest version of FlashList:

Got @j-piasecki's fabricfixture app building properly.
Rebased this fabric branch with @shopify/flash-list@master to bring the library up to the latest version (1.6.2)
Upgraded the fabricfixture app from [email protected] to 0.72.5 ensuring that both iOS and android fixture apps built and displayed the FlashList component with the new architecture turned on.

Things I did NOT do:

I did not touch or edit ANY of the fabric code that @j-piasecki wrote. All credit goes to him.

Things I think we still need to do:

Investigate using react-native-test-app so we don't have to have two fixture apps in the repo anymore (one for new architecture and another for old architecture). This will get rid of a lot of duplicated code as suggested by @fortmarek here Fabric support now uses the main fixture app.
Ensure that nothing broke in the non-fabric renderer.
Test the he🏒🏒 out of this.

Reviewers’ hat-rack 🎩

Check the instructions in the readme for how to test the fixture app in both old and new architecture..

Screenshots or videos (if needed)

These videos are from the app built by the react native app in ./fabricfixture. Get ready to have a seizure, lol.

iOS	Android

Checklist

I have added a changelog entry following the Keep a Changelog guidelines.

markrickert · 2023-10-10T00:24:26Z

I have signed the CLA!

RichardLindhout · 2023-10-26T14:44:28Z

LGTM!

markrickert · 2023-10-26T16:00:54Z

Thanks for testing this out @RichardLindhout! Did you run across any issues? Any ideas on how we could combine the two fixture apps into a single app that could test both the old and new architecture?

RichardLindhout · 2023-10-26T16:15:35Z

I did not test it, I just was hoping FlashList would get Fabric support :). I'm not experienced with Fabric so I'm not sure.

naqvitalha · 2023-11-09T17:25:13Z

Apologies for the delay, I'll try this out soon. Thanks a lot

ios/Sources/AutoLayoutViewComponentView.mm

T-Damer · 2023-11-12T13:36:28Z

Let's goooo

WoLewicki

LGTM 🚀

chakravarthi-bb · 2023-12-06T11:47:29Z

Can we release this??

evelant · 2023-12-14T19:52:17Z

This is the last dependency our app preventing it from working with the new arch. Hopefully this can get released soon!

evelant · 2023-12-14T21:25:36Z

@markrickert I tried your fork in my app and it seems to work perfectly with the new architecture enabled!

markrickert · 2023-12-15T18:35:15Z

I rebased my work here on main.

WoLewicki · 2023-12-18T11:29:35Z

@markrickert could you maybe bump the version of fabric app to RN 0.73 to see if it works?

markrickert · 2023-12-18T16:12:46Z

@markrickert could you maybe bump the version of fabric app to RN 0.73 to see if it works?

That's a great idea. I'll work on that here in the next few days

BLOCKMATERIAL · 2023-12-25T20:53:29Z

up please very important

JJSLIoT · 2024-01-01T05:59:49Z

Can we get this lint check passed to unblock the PR from being approved?

renopp · 2024-01-08T01:41:34Z

Tested at RN 0.73.1 android and running well ❤️

fabricfixture/.eslintrc.js

WoLewicki

Could you add those changes so it works correctly on new arch in the newest version of RN and also makes it possible to use it from this commit?

RNFlashList.podspec

package.json

markrickert · 2024-01-31T21:57:20Z

This PR should be ready to go. I've updated the PR based on suggestions as well as ensured that both the fixture app and fabricfixture app build and run on the latest Mac OS (14.3) and the latest Xcode (15.2)

CI is finally passing all green ✅ and I rebased on main and force-pushed (sorry to anyone using my branch commit hashes directly in their project).

The fabricfixture app has been updated to react-native 0.72.10 for now.

WoLewicki

I left some comments for now. Please answer them.

RNFlashList.podspec

android/build.gradle

android/src/main/kotlin/com/shopify/reactnative/flash_list/AutoLayoutView.kt

WoLewicki · 2024-02-01T13:27:33Z

ios/Sources/AutoLayoutView.swift

                    return cellContainer
+                } else if subview is AutoLayoutView {


So maybe we should make the AutoLayoutView not collapsable to avoid it?

@WoLewicki @markrickert just wanted to check: apart from this comment are there any other issues with this PR which is blocking the merge?

Nope, it looks ok

WoLewicki · 2024-02-01T13:28:34Z

ios/Sources/CellContainerManager.swift

@@ -3,7 +3,7 @@ import Foundation
 @objc(CellContainerManager)
 class CellContainerManager: RCTViewManager {  
    override func view() -> UIView! {
-        return CellContainer()
+        return CellContainerComponentView()


How does it work if there is no implementation of this class on paper? Or am I missing something?

Honestly, this would be a question for @j-piasecki since he made all the changes to the native source code. I didn't delve into exactly how that all works, but both fixture apps are running so maybe some magic codegen going on here? I'm not as well versed in new arch native changes as I'd like to be right now.

Ok I contacted him and I get it now. You can see such code at the end of CellContainerComponentView:

@implementation CellContainerComponentView @end

so it resolves to simple UIView there 😅

markrickert · 2024-04-18T20:44:53Z

Rebased on main and fixed the merge conflicts.

ChronoByteCosmonaut · 2024-04-23T22:34:20Z

Is this going to be merged soon? 🤔

jamonholmgren

Nice work on this, @markrickert! (And @j-piasecki too!)

T-Damer

🚀

anton-patrushev

Great job!

qq: is it compatible with latest 74 RN release?

ChronoByteCosmonaut · 2024-04-26T07:32:32Z

What’s the status on this one?

gorbypark · 2024-04-28T10:21:51Z

Great job!

qq: is it compatible with latest 74 RN release?

Just a very quick test, but I spun up a new Expo 51 project (which is RN 0.74.0) and this pr seems to work great on both iOS/Android with new arch bridgless mode enabled.

# create a new project with SDK51 (as of April 28, 2024 this is the beta version)
yarn create expo-app --template default@beta

# this just removes the examples in the app folder
yarn reset-project

# required to enable new architecture
npx expo install expo-build-properties

# use flashlist from this PR
yarn add https://github.com/markrickert/flash-list.git\#mrickert/fabric

# generate android and ios folders
npx expo prebuild --clean

app.json (enable new architecture)

    "plugins": [
      "expo-router",
      [
        "expo-build-properties",
        {
          "ios": {
            "newArchEnabled": true
          },
          "android": {
            "newArchEnabled": true
          }
        }
      ]
    ],

/app/index.tsx

import React from "react";
import { Text } from "react-native";
import { FlashList } from "@shopify/flash-list";

const DATA = [
{
 title: "First Item",
},
{
 title: "Second Item",
},
];

export default function App() {
return (
 <FlashList
   data={DATA}
   renderItem={({ item }) => <Text>{item.title}</Text>}
   estimatedItemSize={200}
 />
);
}

Build and run app

yarn ios
yarn android

A very simple usage of flashlist (copied directly from the flashlist doc site) but both platforms are working as expected. There seems to be some issue with types when installing from the GitHub repo instead of NPM, but I'm going to assume that will sort itself out if this is merged into main.

almoatamed · 2024-05-09T22:08:15Z

this is very important, is this going to be merged?

almoatamed · 2024-05-09T22:09:30Z

@WoLewicki please your review

WoLewicki · 2024-05-13T13:08:29Z

I am not a maintainer of this library unfortunately.

saxenanickk · 2024-05-16T17:23:49Z

@naqvitalha @jenshenny Anyone maintaining this library?

jfmoe · 2024-05-21T03:25:11Z

Any progress?

naqvitalha · 2024-05-27T15:47:07Z

We've started looking at this PR. I'll update soon. Thanks @markrickert @j-piasecki

naqvitalha · 2024-05-29T16:05:29Z

ios/Sources/AutoLayoutView.swift

@@ -4,31 +4,33 @@ import UIKit

 /// Container for all RecyclerListView children. This will automatically remove all gaps and overlaps for GridLayouts with flexible spans.
 /// Note: This cannot work for masonry layouts i.e, pinterest like layout
-@objc class AutoLayoutView: UIView {
+@objc public class AutoLayoutView: UIView {
+    @objc public var onBlankAreaEventHandler: ((CGFloat, CGFloat) -> Void)?


@markrickert While testing I noticed that onBlankArea event isn't working. On Android, the name of the event was emitBlankAreaEvent instead of onBlankAreaEvent so I fixed it. It also isn't working on iOS, I'm not sure if this is the right way to handle it.

@naqvitalha did you maybe check if it fails to enter this part of code: https://github.com/Shopify/flash-list/pull/931/files#diff-5fac05017cc8297ba991cb968f2703854339d60ae995e08e9cfe66545186f745R46 ? It is the place where the event should be sent to JS.

It seems like the RCT_NEW_ARCH_ENABLED preprocessor directive isn't working as expected. @j-piasecki Can you check?

Jackmekiss · 2024-06-05T09:12:18Z

Let's merge it guys!

j-piasecki · 2024-06-06T06:46:50Z

RNFlashList.podspec

  s.source           = { git: 'https://github.com/shopify/flash-list.git', tag: "v#{s.version}" }
  s.source_files     = 'ios/Sources/**/*'
  s.requires_arc     = true
  s.swift_version    = '5.0'

-  # Dependencies
-  s.dependency 'React-Core'
+  # Use install_modules_dependencies helper to install the dependencies if React Native version >=0.71.0.


@naqvitalha The RCT_NEW_ARCH_ENABLED isn't actually set anywhere.

Suggested change

# Use install_modules_dependencies helper to install the dependencies if React Native version >=0.71.0.

if fabric_enabled

s.pod_target_xcconfig = {

'OTHER_SWIFT_FLAGS' => '-D RCT_NEW_ARCH_ENABLED',

}

end

# Use install_modules_dependencies helper to install the dependencies if React Native version >=0.71.0.

And then it would also be required to check if the flag is set at the top level (cannot add suggestion there 😞):

fabric_enabled = ENV['RCT_NEW_ARCH_ENABLED']

markrickert · 2024-06-18T21:36:45Z

Closing because #550 was merged.

github-actions bot added the cla-needed label Oct 10, 2023

github-actions bot removed the cla-needed label Oct 10, 2023

markrickert force-pushed the mrickert/fabric branch from 5ca942c to 1b7533b Compare October 10, 2023 22:29

Flewp added a commit to discord/flash-list that referenced this pull request Oct 11, 2023

Squash commit of Shopify#931

0d5a18d

markrickert mentioned this pull request Oct 11, 2023

feat(boilerplate) @shopify/flash-list integration infinitered/ignite#2513

Merged

2 tasks

WoLewicki reviewed Nov 10, 2023

View reviewed changes

ios/Sources/AutoLayoutViewComponentView.mm Outdated Show resolved Hide resolved

WoLewicki approved these changes Nov 13, 2023

View reviewed changes

markrickert force-pushed the mrickert/fabric branch from e819396 to 05ab455 Compare December 15, 2023 18:34

T-Damer reviewed Jan 8, 2024

View reviewed changes

fabricfixture/.eslintrc.js Outdated Show resolved Hide resolved

WoLewicki suggested changes Jan 17, 2024

View reviewed changes

RNFlashList.podspec Outdated Show resolved Hide resolved

RNFlashList.podspec Outdated Show resolved Hide resolved

package.json Show resolved Hide resolved

markrickert force-pushed the mrickert/fabric branch from 11c0e20 to 32a7317 Compare January 31, 2024 21:53

markrickert requested a review from WoLewicki January 31, 2024 21:57

WoLewicki reviewed Feb 5, 2024

View reviewed changes

j-piasecki mentioned this pull request Feb 7, 2024

New architecture support #550

Merged

1 task

markrickert added 5 commits April 18, 2024 12:17

Linter Fixes

c1ba66a

More linter fixes.

dbcc26b

Attempt to fix CI Pipelines.

cc4f6a7

Trying to get ios to build in CI

c9f7534

Maybe static frameworks will build?

195d1f5

markrickert force-pushed the mrickert/fabric branch from 02990f2 to 195d1f5 Compare April 18, 2024 20:44

markrickert requested review from anton-patrushev and WoLewicki April 18, 2024 21:02

TheLonelyAstronaut approved these changes Apr 24, 2024

View reviewed changes

jamonholmgren approved these changes Apr 24, 2024

View reviewed changes

T-Damer approved these changes Apr 25, 2024

View reviewed changes

anton-patrushev approved these changes Apr 25, 2024

View reviewed changes

hungvu193 mentioned this pull request May 21, 2024

Fix: Unable to select emoji if searching for specific one Expensify/App#42407

Merged

50 tasks

fix blank area event name

41eaca0

naqvitalha reviewed May 29, 2024

View reviewed changes

j-piasecki reviewed Jun 6, 2024

View reviewed changes

nyagami mentioned this pull request Jun 13, 2024

LNReader Fabric (new React Native architecture) LNReader/lnreader#1046

Open

26 tasks

markrickert closed this Jun 18, 2024

-  # Use install_modules_dependencies helper to install the dependencies if React Native version >=0.71.0.
+  if fabric_enabled
+    s.pod_target_xcconfig = {
+      'OTHER_SWIFT_FLAGS' => '-D RCT_NEW_ARCH_ENABLED',
+    }
+  end
+  # Use install_modules_dependencies helper to install the dependencies if React Native version >=0.71.0.

Support for New Architecture - iOS & Android (Fabric) #931

Support for New Architecture - iOS & Android (Fabric) #931

Conversation

markrickert commented Oct 10, 2023 • edited Loading

Description

Here's what I did to bring things back up to date so that people can start testing this with the latest version of FlashList:

Things I did NOT do:

Things I think we still need to do:

Reviewers’ hat-rack 🎩

Screenshots or videos (if needed)

Checklist

markrickert commented Oct 10, 2023

RichardLindhout commented Oct 26, 2023

markrickert commented Oct 26, 2023

RichardLindhout commented Oct 26, 2023

naqvitalha commented Nov 9, 2023

T-Damer commented Nov 12, 2023

WoLewicki left a comment

Choose a reason for hiding this comment

chakravarthi-bb commented Dec 6, 2023

evelant commented Dec 14, 2023

evelant commented Dec 14, 2023 • edited Loading

markrickert commented Dec 15, 2023

WoLewicki commented Dec 18, 2023

markrickert commented Dec 18, 2023

BLOCKMATERIAL commented Dec 25, 2023

JJSLIoT commented Jan 1, 2024

renopp commented Jan 8, 2024

WoLewicki left a comment

Choose a reason for hiding this comment

markrickert commented Jan 31, 2024

WoLewicki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markrickert commented Apr 18, 2024

ChronoByteCosmonaut commented Apr 23, 2024

jamonholmgren left a comment • edited Loading

Choose a reason for hiding this comment

T-Damer left a comment

Choose a reason for hiding this comment

anton-patrushev left a comment

Choose a reason for hiding this comment

ChronoByteCosmonaut commented Apr 26, 2024

gorbypark commented Apr 28, 2024 • edited Loading

almoatamed commented May 9, 2024

almoatamed commented May 9, 2024

WoLewicki commented May 13, 2024

saxenanickk commented May 16, 2024

jfmoe commented May 21, 2024

naqvitalha commented May 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackmekiss commented Jun 5, 2024

Choose a reason for hiding this comment

markrickert commented Jun 18, 2024

markrickert commented Oct 10, 2023 •

edited

Loading

evelant commented Dec 14, 2023 •

edited

Loading

jamonholmgren left a comment •

edited

Loading

gorbypark commented Apr 28, 2024 •

edited

Loading