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

Fix iOS Asset Linking #1232

Closed
wants to merge 1 commit into from
Closed

Fix iOS Asset Linking #1232

wants to merge 1 commit into from

Conversation

kylerjensen
Copy link

@kylerjensen kylerjensen commented Aug 25, 2020

Because RNVectorIcons.podspec includes a resources line AND react-native-config.js includes an assets line, each font file is being copied into the iOS project twice when using react native >= 0.60 (which supports autolinking) and running react-native link. This commit removes the resources line from the podspec, making it so that just a single copy of the assets are added to the project. I chose to remove it from the podspec rather than the react-native config because the react-native config also copies assets for Android and I didn't want to break that functionality.

Closes #1173

Because `RNVectorIcons.podspec` includes a `resources` line AND `react-native-config.js` includes an `assets` line, each font file is being copied into the iOS project twice when using react native >= 0.60 (autolinking). This commit removes the `resources` line from the podspec, making it so that just a single copy of the assets are added to the project. I chose to remove it from the podspec, not the react-native config because the react-native config also copies assets for Android and I didn't want to break that functionality.
@mrousavy
Copy link
Contributor

+1

@oblador
Copy link
Owner

oblador commented Sep 10, 2020

This broke the example application when I applied the changes 🤔

@kylerjensen
Copy link
Author

kylerjensen commented Sep 10, 2020

@oblador Thanks for reviewing the PR. Are you talking about the TabBarExample? If so, the breakage you're seeing is likely because the example is still on react-native@0.57.1, which doesn't have support for auto linking, so you wouldn't have seen this issue or the fix unless you explicitly tried to run react-native link:

This library is currently broken on react-native@>=0.60 and needs a fix, but I don't know how to do that and still keep backward-compatibility with react-native@<0.60. I think this is going to need to be released as a semver: major (breaking change).

With the IconExplorerExample, you would also see the issue and the fix if you run react-native link.

With your blessing, I'll make the necessary changes in this PR to prep for a major version update:

  • Bump the version of react-native used in both of the examples to at least 0.60
  • Update the README, requiring users to run npx react-native link after installing this library
  • Update the README, instructing users to install react-native-vector-icons@7 if they still want to use react-native@<0.60

Does that work for you? Any other changes?

@mrousavy
Copy link
Contributor

+1 on the README updates. It's really confusing, especially for newcomers, to install a native module when it really is just a single command. No need to explain complex steps like link command or manual linking.

@kylerjensen
Copy link
Author

kylerjensen commented Sep 10, 2020

Now that I'm reading through the Autolinking docs again, I think the title of this PR might be misleading. What I actually did was fix support for react-native link and broke support for installation via cocoapods only (and as a result, "Autolinking"). I think another solution might be to flip this PR by removing react-native-config.js and leaving the resources line in the Podspec. That would support true Autolinking (as defined here: https://github.com/react-native-community/cli/blob/master/docs/autolinking.md), and preserve support for installation via cocoapods, however it will require users to modify their Info.plist files whereas react-native link takes care of registering custom fonts automatically. I'm going to try it out the other way to see what happens; stay tuned.

@kylerjensen kylerjensen changed the title Fix iOS Autolinking Fix iOS Asset Linking Sep 10, 2020
@kylerjensen kylerjensen marked this pull request as draft September 10, 2020 23:18
@danielfx90
Copy link

@kylerjensen any news on your last approach? It sounds interesting and might solve a lot of headaches

@kylerjensen
Copy link
Author

@danielfx90 I put this on hold for a while because I'm not sure which direction to go. Either we fix autolinking and force devs to manually add fonts to their Info.plists, or we fix react-native link — seems we can't have one without the other. In the meantime, this is the patch we've been using (with patch-package) that fixes react-native link behavior:

diff --git a/node_modules/react-native-vector-icons/RNVectorIcons.podspec b/node_modules/react-native-vector-icons/RNVectorIcons.podspec
index 9a1d11d..ff2e9ad 100644
--- a/node_modules/react-native-vector-icons/RNVectorIcons.podspec
+++ b/node_modules/react-native-vector-icons/RNVectorIcons.podspec
@@ -12,7 +12,6 @@ Pod::Spec.new do |s|
   s.platforms      = { :ios => "9.0", :tvos => "9.0" }
   s.source         = { :git => "https://github.com/oblador/react-native-vector-icons.git", :tag => "v#{s.version}" }
   s.source_files   = 'RNVectorIconsManager/**/*.{h,m}'
-  s.resources      = "Fonts/*.ttf"
   s.preserve_paths = "**/*.js"
   s.dependency 'React'
 

@danielfx90
Copy link

@danielfx90 I put this on hold for a while because I'm not sure which direction to go. Either we fix autolinking and force devs to manually add fonts to their Info.plists, or we fix react-native link — seems we can't have one without the other. In the meantime, this is the patch we've been using (with patch-package) that fixes react-native link behavior:

diff --git a/node_modules/react-native-vector-icons/RNVectorIcons.podspec b/node_modules/react-native-vector-icons/RNVectorIcons.podspec
index 9a1d11d..ff2e9ad 100644
--- a/node_modules/react-native-vector-icons/RNVectorIcons.podspec
+++ b/node_modules/react-native-vector-icons/RNVectorIcons.podspec
@@ -12,7 +12,6 @@ Pod::Spec.new do |s|
   s.platforms      = { :ios => "9.0", :tvos => "9.0" }
   s.source         = { :git => "https://github.com/oblador/react-native-vector-icons.git", :tag => "v#{s.version}" }
   s.source_files   = 'RNVectorIconsManager/**/*.{h,m}'
-  s.resources      = "Fonts/*.ttf"
   s.preserve_paths = "**/*.js"
   s.dependency 'React'
 

@kylerjensen I think autolinking is the new standard. Manual linking is deprecated.
Autolinking with further steps is something that other libraries face as well and should be the way to go.

Cheers

@hackrx
Copy link

hackrx commented Dec 6, 2020

I am still getting this error:-

yarn run v1.22.10
$ react-native run-ios
error React Native CLI uses autolinking for native dependencies, but the following modules are linked manually: 
  - react-native-vector-icons (to unlink run: "react-native unlink react-native-vector-icons")
This is likely happening when upgrading React Native from below 0.60 to 0.60 or above. Going forward, you can unlink this dependency via "react-native unlink <dependency>" and it will be included in your app automatically. If a library isn't compatible with autolinking, disregard this message and notify the library maintainers.

After removing s.resources = "Fonts/*.ttf" from node_modules/react-native-vector-icons/RNVectorIcons.podspec
Any idea how to solve this issue?
If I run react-native link then react-native unlink react-native-vector-icons
Then it doesn’t work for android but this error is removed.

@kylerjensen
Copy link
Author

Closing this for now. I've come up with a suitable workaround (see above), but implementing/fixing auto linking is beyond the scope of what I have time for right now. If someone else wants to try their hand at auto linking, I'm sure it would be greatly appreciated.

@kylerjensen kylerjensen deleted the patch-1 branch February 17, 2021 18:51
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.

React native vector icons linking
5 participants