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

Ability to skip autolinking for dependency for specific ios target #1179

Closed
anta-semenov opened this issue May 27, 2020 · 14 comments
Closed

Comments

@anta-semenov
Copy link

anta-semenov commented May 27, 2020

Describe the Feature

We use the same codebase for different apps in appstore, so we have different targets in pod file. For some of the apps we need to remove specific dependencies (for instance, we can't use firebase admob library in target that is aiming for kids due to appstore policy).

Currently in react-native.config.js I can disable autolinking only for specific platfrom, but it would be good to be able to skip dependency for specific pod targets as well

Possible Implementations

I can imagine that it could be a specific property in react-native.config.jsin dependency configuration where I can putt targets that should be excluded from autolinking

module.exports = {
  assets: ['res/fonts'],
  dependencies: {
    'react-native-code-push': {
      platforms: {
        ios: {
          excludeFor: ['Target1'],
        },
        android: null,
        web: null,
      }
    }
  }
}

I think in ../node_modules/@react-native-community/cli-platform-ios/native_modules it could be possible to get this property for dependency and skip installation if current target is listed

@thymikee
Copy link
Member

cc @grabbou @alloy

@alloy
Copy link
Member

alloy commented May 27, 2020

In my opinion, which is very strong when it comes to options/configuration 😅, auto-linking should be as simple as it can be without the need for configuration. Any further customisation should be done entirely through normal CocoaPods [or Gradle] means.

The reason I feel this way is that I think it ends up hurting the user if we try to abstract away things that are already well covered by the amount of docs/issues/stackoverflow/blogs that exist for the underlying tool–in this case CocoaPods. Instead we would have to write and maintain new rn-cli specific documentation, meaning maintainers need to do more work and end-users have less chance of finding the right solution; which is a lose-lose situation.

In short, I think auto-linking should:

  1. not ever try include dependencies on platforms the pod doesn’t support (added in feat(cli-platform-ios): add macOS auto-linking support #1126)
  2. auto-linking should skip pods that were already explicitly activated by the user prior to invoking auto-linking, thus allowing the user to provide custom configuration for that pod (already works this way)
  3. auto-linking can be invoked from within specific targets, thus not affecting other targets (should already work this way)

Thus, I don’t think we should add this feature, furthermore I believe we should remove the ability to exclude pods for specific platforms. It would also be good to outline the above design in the auto-linking doc.

@alloy
Copy link
Member

alloy commented May 27, 2020

@anta-semenov So coming back to your original issue, I think your best recourse would be to follow the strategy outlined in point 3 above; which is to:

  • invoke use_native_modules! in the targets where you want auto-linking to do its work
  • manually define the pods in those targets where you want total control over them.

If this doesn't work, then please let us know! 🙏

@anta-semenov
Copy link
Author

anta-semenov commented May 28, 2020

Not sure if this will work well with my user case.
My use case is following (pod file):

abstract_target 'Main' do
  <React native pods>
  <Common dependencies, around 20>

  target InAppAddAllowed do
    pod pod 'RNFBAdMob', :path => '../node_modules/@react-native-firebase/admob'
  end
  target AppWithoutAdd
end

I would like to use autolinking to install common dependencies, but according to my understanding if I add use_native_modules! into Main target level it will also add RNFBAdMob, because it will find it in node_modules (or maybe I'm wrong in my understanding how the whole thing work). I can use use_native_modules! only inside InAppAddAllowed to add one dependency which obviously doesn't have any sense.
And it's not very easy to remove dependency from target with cocoapods (at least there is no docs around this), so I can't use cocopods hooks for that

But I got your point. Will patch this locally.

I have one question in how merging of react-native.config.js works.
Is there any merging at all, meaning if I define dependencies in my local react-native.config.js and there is already some config in package, will it merge configs or will it select only one of them?
And if it merges, will it merge only known properties or all of them?

And in my proposal, I didn't mean that there should be the need for configuration, but ability for configuration

@alloy
Copy link
Member

alloy commented May 28, 2020

Can you check what this does?

abstract_target 'Main' do
  <Common dependencies, around 20>

  target "InAppAddAllowed" do
    pod 'RNFBAdMob', :path => '../node_modules/@react-native-firebase/admob'
    use_native_modules!
  end

  target "AppWithoutAdd" do
    use_native_modules!
  end
end

If this does not already omit RNFBAdMob from the AppWithoutAdd target, then we should update it to do so, as that’s the way I would prefer configuration to be done.

And in my proposal, I didn't mean that there should be the need for configuration, but ability for configuration

To be clear, my first comment wasn’t necessarily addressed at you, more so to the other maintainers of this tool and for posterity.

I think we are on the same page, we want to be able to control these things in as easy a way as possible.

@anta-semenov
Copy link
Author

Will try today or on monday. But according to code in @react-native-community/cli-platform-ios/native_modules.rb it checks dependencies only for current target, so inside AppWithoutAdd it most likely won't check dependencies of InAppAddAllowed (or maybe I completely don't understand how cocoapods works)

@Charlie-Hua
Copy link

I ended up patching native_modules.rb to take packages_to_skip in use_native_modules!, so I can do this in my Podfile:

abstract_target 'Main' do
  # common dependencies, around 20
  use_native_modules!(packages_to_skip: ['RNFBAdMob'])

  target "InAppAddAllowed" do
    pod 'RNFBAdMob', :path => '../node_modules/@react-native-firebase/admob'
  end

  target "AppWithoutAdd" do
  end
end

here is the content of my patches/@react-native-community+cli-platform-ios+4.13.0.patch file

diff --git a/node_modules/@react-native-community/cli-platform-ios/native_modules.rb b/node_modules/@react-native-community/cli-platform-ios/native_modules.rb
index 2254158..397ca0a 100644
--- a/node_modules/@react-native-community/cli-platform-ios/native_modules.rb
+++ b/node_modules/@react-native-community/cli-platform-ios/native_modules.rb
@@ -12,7 +12,7 @@
 require 'pathname'
 require 'cocoapods'
 
-def use_native_modules!(config = nil)
+def use_native_modules!(config = nil, packages_to_skip: [])
   if (config.is_a? String)
     Pod::UI.warn("Passing custom root to use_native_modules! is deprecated.",
       [
@@ -44,6 +44,10 @@ def use_native_modules!(config = nil)
 
   packages.each do |package_name, package|
     next unless package_config = package["platforms"]["ios"]
+    if skipped_pod = packages_to_skip.find { |pod_name| package["platforms"]["ios"]["podspecPath"].include? "#{pod_name}.podspec" }
+      Pod::UI.notice "Skipping pod: #{skipped_pod}"
+      next
+    end
 
     podspec_path = package_config["podspecPath"]
 

@stockhuman
Copy link

Hey @Charlie-Hua, that's a great idea. This should make it to the canonical implementation; as niche as the the feature may be, it's incredibly useful for simulator vs real-device-only pods & separation of concerns. We use to it to split out our Unity framework interop. Thanks!

@tapi
Copy link

tapi commented Mar 10, 2022

The skip list is exactly what we need. We have a dependency required only for Android but it has an iOS native module that we don't want. Generally I agree with @alloy, configuration should be easy, but I don't think an exclusion list is at odds with that.

@pot2mayo
Copy link

pot2mayo commented Mar 14, 2022

Note that this feature would also be extremely useful when creating an iOS app clip!
App clips have strong size constraints (10MB uncompressed), therefore being able to filter out packages that you don't need in the clip (which must have its own target in the Podfile) would be great since it cannot be done in the package.json which is shared between the app and the clip.

For now I'm gonna try @Charlie-Hua's solution which is a very nice workaround this limitation 👍🏼

@Ross-Landry
Copy link

@pot2mayo Did that work for you?

I am trying to reduce my app clip size as well. I tried the work-around but I did not see a reduction in size. (I also didn't see any messages in the console about pods being skipped, so I'm not sure that my attempt to copy the patch worked.) Curious if you had success in this.

@github-actions
Copy link

There hasn't been any activity on this issue in the past 3 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.

@sa-ma
Copy link

sa-ma commented Feb 20, 2024

@pot2mayo Did that work for you?

I am trying to reduce my app clip size as well. I tried the work-around but I did not see a reduction in size. (I also didn't see any messages in the console about pods being skipped, so I'm not sure that my attempt to copy the patch worked.) Curious if you had success in this.

@Ross-Landry
Were you able to solve this?

@kecoco16
Copy link

@sa-ma To reduce the App clip, you can use this approach. This way you can use the same base code for both the app and the app clip and only install the Pods necessary for the app clip and in this way not increase the size of the app clip, thanks to @dawidzawada for sharing this approach 💪💪

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

No branches or pull requests

10 participants