search

DylanVann / react-native-fast-image

🚩 FastImage, performant React Native image component.
MIT License
8.09k stars 1.47k forks source link

feat: add Fabric support #951

Open WoLewicki opened 1 year ago

WoLewicki commented 1 year ago

Add Fabric support to the library. Some of the code has been provided in https://github.com/DylanVann/react-native-fast-image/pull/933 :tada:

andresribeiro commented 1 year ago

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

samjayhk commented 1 year ago

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

norbusonam commented 1 year ago

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 commented 1 year ago

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

Rexogamer commented 1 year ago

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

WoLewicki commented 1 year ago

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 commented 1 year ago

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

tomgreco commented 1 year ago

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

tomgreco commented 1 year ago

@WoLewicki when I updated my package.json to point to your fork I had to patch the package.json to make it work. Without the prepare: npm run build step there was never a dist folder generated.

I'm pretty sure this is a specific issue related to using a github url in a package.json but still worth mentioning.

numandev1 commented 1 year ago

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 commented 1 year ago

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

numandev1 commented 1 year ago

@WoLewicki thanks a lot for your efforts <3

WoLewicki commented 1 year ago

@numandev1 can you check if it works now?

numandev1 commented 1 year ago

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

billnbell commented 1 year ago

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

billnbell commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

billnbell commented 11 months ago

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

Will this work without using expo?

tsapeta commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

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

chj-damon commented 11 months ago

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

ajeetfancode commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

billnbell commented 11 months ago

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

I am unsure :)

ajeetfancode commented 11 months ago

@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 commented 11 months ago

I think you are right use component names