Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add Fabric support #951

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

WoLewicki
Copy link

@WoLewicki WoLewicki commented Nov 30, 2022

PR adding New Architecture support to the library 🎉

We at Software Mansion have been working on improving support for the new architecture for quite a while now. If you need help with anything related to New Architecture, like:

or you just want to ask any questions, hit us up on [email protected]


Notes

Some of the code has been provided in #933 🎉

@andresribeiro
Copy link

@DylanVann i know this library is, in your own words on twitter, "slowly maintained", but could you take a look at this?

@samjayhk
Copy link

samjayhk commented Mar 1, 2023

Hello @DylanVann , may I know if you will consider this PR as one of the support of new architects for this library ? Thanks.

@norbusonam
Copy link

Does anyone know if this PR is still being considered? Working on a project using the new architecture so this would be great for me

@canpoyrazoglu
Copy link

I'd love to see this PR upstream if everything works correctly!

@Rexogamer
Copy link

While we wait for a response from Dylan, would you be willing to publish this fork as its own package?

@WoLewicki
Copy link
Author

What is a benefit of making a fork of this package? You can always install code from a commit. Or am I missing something @Rexogamer ?

@Rexogamer
Copy link

Oh, true - I was more suggesting publishing the existing fork as its own package, but installing from a commit should work fine

@tomgreco
Copy link

What are the steps to use this particular commit (which commit?)

@numandev1
Copy link
Contributor

i added a folder build in the repo and installing like this and it is working but getting this error Error: Unsupported top level event type "topOnFastImageLoadStart" dispatched, js engine: hermes

    "react-native-fast-image": "https://github.com/numandev1/react-native-fast-image#feat/fabric-wolewicki",

@WoLewicki
Copy link
Author

@numandev1 yes, it is a problem in RN 0.72. I'll push the commits fixing it soon.

@numandev1
Copy link
Contributor

@WoLewicki thanks a lot for your efforts <3

@WoLewicki
Copy link
Author

@numandev1 can you check if it works now?

@numandev1
Copy link
Contributor

@WoLewicki now it is working well, thanks a lot <3

@billnbell
Copy link

Can someone please fork this and maintain it - call it "react-native-fast-image2" ?

@billnbell
Copy link

OK for use_framework! this change it needed on RN 0.72.1 for some reason you need to add React-debug and React-utils.

Try this.
use_frameworks! :linkage => :static

react-native-fast-image+8.6.3.patch
diff --git a/node_modules/react-native-fast-image/RNFastImage.podspec b/node_modules/react-native-fast-image/RNFastImage.podspec
index 7e50f18..8faf996 100644
--- a/node_modules/react-native-fast-image/RNFastImage.podspec
+++ b/node_modules/react-native-fast-image/RNFastImage.podspec
@@ -27,12 +27,29 @@ Pod::Spec.new do |s|
     s.source_files    = 'ios/**/*.{h,m,mm,cpp}'
 
     s.dependency "React"
+    s.dependency "React-debug"
+    s.dependency "React-utils"
     s.dependency "React-RCTFabric"
     s.dependency "React-Codegen"
     s.dependency "RCT-Folly"
     s.dependency "RCTRequired"
     s.dependency "RCTTypeSafety"
     s.dependency "ReactCommon/turbomodule/core"
+
+    s.subspec "xxxdebug" do |ss|
+      ss.dependency "ReactCommon"
+      ss.dependency "React-debug"
+      ss.source_files         = "react/debug/**/*.{cpp,h}"
+      ss.header_dir           = "react/debug"
+      ss.pod_target_xcconfig  = { "HEADER_SEARCH_PATHS" => "\"${PODS_CONFIGURATION_BUILD_DIR}/React-debug/React_debug.framework/Headers\"" }
+    end
+    s.subspec "xxxutils" do |ss|
+      ss.dependency "ReactCommon"
+      ss.dependency "React-utils"
+      ss.source_files         = "react/utils/**/*.{cpp,h}"
+      ss.header_dir           = "react/utils"
+      ss.pod_target_xcconfig  = { "HEADER_SEARCH_PATHS" => "\"${PODS_CONFIGURATION_BUILD_DIR}/React-utils/React_utils.framework/Headers\"" }
+    end
   else
     s.platforms     = { :ios => "8.0", :tvos => "9.0" }
     s.source_files  = "ios/**/*.{h,mm}"

@numandev1
Copy link
Contributor

numandev1 commented Jul 7, 2023

@WoLewicki can you maintain this library by forking and pushing with another name on npm? because the owner of this library not having time to see it

@WoLewicki
Copy link
Author

@numandev1 you can always use expo-image which is compatible with Fabric already.

@billnbell
Copy link

@numandev1 you can always use expo-image which is compatible with Fabric already.

Will this work without using expo?

@tsapeta
Copy link

tsapeta commented Jul 17, 2023

@billnbell You only need to install and configure Expo modules with npx install-expo-modules@latest script (follow https://docs.expo.dev/bare/installing-expo-modules/ guide). Once you do that, you can still use RN CLI if that's what you prefer.

@chj-damon
Copy link

@numandev1 you can always use expo-image which is compatible with Fabric already.

so you recommend to use expo-image over this library? does that mean you're gonna abandon this PR for now? I don't like to install expo-modules only if I want to handle Image things.

@chj-damon
Copy link

@WoLewicki It would be nice if you can maintain this as a new package, just like react-native-blob-util which replaces rn-fetch-blob since rn-fetch-blob has not been maintained.

@tsapeta
Copy link

tsapeta commented Jul 24, 2023

@chj-damon May I ask what's stopping you from installing Expo modules?
The impact in binary size is now marginal (and will be even smaller in the next SDK), especially compared to Hermes which surprisingly nobody complains about.
Expo modules are also very well maintained by two companies, including us at @software-mansion.
They all work with the New Architecture and use JSI even on the old one, making native calls much faster. We also provide some support on the Expo Developers discord in case you have any questions. If you used Expo in the past, I would recommend you to give it another try, a lot have changed.

@chj-damon
Copy link

@tsapeta

  1. expo modules require ios 13 above, which is not ok with my app.
  2. expo will install so many things like expo-asset/ expo-constants/ expo-font etc... which I don't want to use. All I want is a tree, but it gives me a whole forest.
  3. I need to be campatible with some lower react-native versions, like 0.64.x, which it will fails with expo modules.

@tsapeta
Copy link

tsapeta commented Jul 24, 2023

@chj-damon

  1. React Native 0.73.x will require even 13.4, so I don't think that's a big deal and you should rather drop support for older versions (that's a standard now). iOS below 13.0 is used by less than 1% of active App Store users.
  2. These dependencies are very small and are necessary for many other modules. I'm working on decoupling them, so in SDK 50 they won't be installed.
  3. If you need to be compatible with 0.64.x, then why do you want the FastImage with Fabric support?

@chj-damon
Copy link

@tsapeta thanks for your reply. I'll give it a shot.

@chj-damon
Copy link

@tsapeta hey, I just want to tell you that I've tried expo-image and it works so good! Thank you so much!

@ajeetfancode
Copy link

i added a folder build in the repo and installing like this and it is working but getting this error Error: Unsupported top level event type "topOnFastImageLoadStart" dispatched, js engine: hermes

    "react-native-fast-image": "https://github.com/numandev1/react-native-fast-image#feat/fabric-wolewicki",

I am getting the same error, I have added "FastImageView" in react-native.config.js, was trying out the New Renderer Interop Layer
project: { ios: { unstable_reactLegacyComponentNames:[ "FastImageView", ] }, android: { unstable_reactLegacyComponentNames:[ "FastImageView", ] }, }

@billnbell
Copy link

Create a file react-native.config.js

module.exports = {
  project: {
    ios: {
      unstable_reactLegacyComponentNames: [
        'react-native-fast-image',
        'CellContainer',
        'AutoLayoutView',
      ],
    },
    android: {},
  },
  assets: ['./src/assets/fonts'],
};

@ajeetfancode
Copy link

@billnbell Isn't we are supposed to give component name instead of the package name in unstable_reactLegacyComponentNames?

@billnbell
Copy link

@billnbell Isn't we are supposed to give component name instead of the package name in unstable_reactLegacyComponentNames?

I am unsure :)

@ajeetfancode
Copy link

@billnbell Isn't we are supposed to give component name instead of the package name in unstable_reactLegacyComponentNames?

I am unsure :)

For some reason I am getting this error :/
error: use of undeclared identifier 'native' providerRegistry->add(concreteComponentDescriptorProvider<UnstableLegacyViewManagerInteropComponentDescriptor<react-native-fast-image>>());

@billnbell
Copy link

I think you are right use component names

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.