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

[Feature]: drop the requirement to install @react-native-vector-icons/common #1682

Open
vonovak opened this issue Dec 11, 2024 · 10 comments
Open

Comments

@vonovak
Copy link
Contributor

vonovak commented Dec 11, 2024

Is your feature request related to a problem?

My suggestion is to simplify installation instructions by not requiring to install the common package.

Describe the solution you'd like

Current state

The common package exposes two utility functions which are available on the individual Icon components. Historically (if I remember correctly), these would mostly be used with legacy components in React Native core which no longer exists such as ToolbarAndroid and TabBarIOS.

AFAICT, the use cases for these functions are limited these days, and 99% of users don't need them. The functions could be extracted into another package where people who do need them could use them from there.

Pros and cons

The benefit of doing this is removal of native code from the typical usage path, and not requiring users to install the extra package - the individual icon packages would still depend on it, and package manager would install it. (The reason it's not done this way already now is probably because React Native CLI tooling links only the native modules which are listed in the root package.json file - so installing it explicitly is now a requirement.). This would make usage easier.

The only downside AFAICT is that it's a breaking change on an API that has been around for a long time.

Describe alternatives you've considered

No response

Additional context

No response

@brentvatne
Copy link

i think they'd be a great fit for a separate package related to font handling, such as expo-font. one change we could make here that would be less disruptive is to move them to a react-native-vector-icons subpackage like @react-native-vector-icons/get-image-source - so people could install this if they need those utils, and as @vonovak pointed out, we can then just make the common package a dependency of each vector-icons subpackage so we don't require that people install @react-native-vector-icons/common just to use a single icon family

@johnf
Copy link
Collaborator

johnf commented Dec 14, 2024

I like this general idea as it solves for the 99%.
I sort of wish we'd thought of it before the 11.0 release 😄

@oblador Any thoughts?

I might have a go at making this work over xmas. We haven't sent any general announcements about the new release yet so we could squeeze this in before we do that.

@oblador
Copy link
Owner

oblador commented Dec 16, 2024

Fun fact, when I originally wrote this lib one of my goal was not requiring a native library - which is still the case only autolinking kinda made this an opt out rather than opt in.

Question is, for non-expo cases, we kinda still want it for ease of integration right @johnf?

For the API part, I think we can keep it as is but using an optional import and throwing an error if not present, just like we do if native module isn’t linked.

@johnf
Copy link
Collaborator

johnf commented Dec 16, 2024

@oblador I had a quick look the other day and the only native code still left is getImageForFont and getImageForFontSync. So thinking along the lines above I think we could

  • Remove the native code from common and only have the common js as a straight dependency
  • Move the code into get-image
  • Update the docs to not require installing common
  • Update the docs to tell folks using getImageForFont that they need to install an extra node module
  • Modify getImageForFont to throw an error if get-image is not installed

@johnf
Copy link
Collaborator

johnf commented Dec 26, 2024

@vonovak @brentvatne I was thinking about this some more this week, and I realised I forgot something, so I'm not convinced this is a good idea anymore.

The common package is also responsible for copying the fonts. It detects which font packages are installed and copies them to the right place. It does this via Gradle on Android and script_phase on iOS.

This functionality is essential for most use cases, especially on Android, as it makes the whole setup zero-touch once you install the packages.

Thoughts?

@brentvatne
Copy link

@johnf - could each icon package be responsible for copying its own font?

@johnf
Copy link
Collaborator

johnf commented Jan 9, 2025

@johnf - could each icon package be responsible for copying its own font?

That's an excellent point. The only downside is that they become a "native module", but that's not a huge deal.

I'll give this a whirl over the weekend.

@brentvatne
Copy link

awesome! sorry for the delayed response, most of the expo team were ooo during holidays and we just got back on monday!

@vonovak
Copy link
Contributor Author

vonovak commented Feb 5, 2025

hello @johnf, just wondering if there's any news on your opinion on each icon package being responsible for copying its own font?

We can help with getting that done.

Thanks :)

@johnf
Copy link
Collaborator

johnf commented Feb 9, 2025

@vonovak Thanks for the reminder, it encouraged me to work on this for the past few days.

I have the start of a pull request at #1711

Current status:

  • Works fine for simple fonts like ant-design
  • More work is needed where the user supplies the font e.g. fontawesomeX-pro, icomoon and fontello
  • Some other things to consider in TODO-no-common.md

It would be great if you could review.

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

No branches or pull requests

4 participants