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

Transitive Dependency Bundling Issue: react-native-paper / react-native-vector-icons #7424

Closed
fbartho opened this issue Mar 20, 2018 · 7 comments
Assignees

Comments

@fbartho
Copy link

fbartho commented Mar 20, 2018

Current behaviour

React-Navigation-Tabs includes react-native-paper as a direct dependency. It only uses this dependency in createMaterialBottomNavigator. react-native-paper includes react-native-vector-icons as a peerDependency but a simple app instantly crashes with a bundler error if react-native-vector-icons is not included in the simple app's package.json

If you include react-native-vector-icons, and run react-native link your app ends up getting all the vector-icon Font-assets linked in, and shipped as part of your app bundle.

Expected behaviour

Since I don't expect to use MaterialBottomNavigator, I wouldn't expect to have a bundler crash complaining that the react-native-vector-icons isn't installed. I also wouldn't expect that using react-navigation-tabs forces me to add vector icons.

What have you tried

Workaround:

  • add react-native-vector-icons as a dependency
  • constantly discard the .ttf file additions to the app bundle / xcodeproj / assets (maybe via post-install script)

Your Environment

software version
ios 11
react 16.3.0-alpha.2
react-native 0.54.2
react-navigation 2.0.0-beta.3
react-navigation-tabs 0.1.0-alpha.1
node 9.7.1
npm / yarn 5.6.0 / 1.5.1
@brentvatne
Copy link
Member

hey @fbartho! I think that in the ideal world we would split up react-native-paper with yarn workspaces/lerna, put BottomNavigation in a separate package, and have the react-native-paper exported version provide some default icon class to it. Do you use createMaterialTopNavigator by any chance? Also, any tips on what we should do here?

@fbartho
Copy link
Author

fbartho commented Mar 21, 2018

Hey @brentvatne!
I haven't used either of the Material* navigators, our app's design is not material (nor iOS)-based.

I agree that the current world of tooling would encourage splitting into sub-packages:

  • react-navigation-tab-tools -- Shared abstractions, the tab router etc...
  • react-navigation-material-tabs -- depends on tab-tools, react-native-paper & transitive deps.
  • react-navigation-anti-material-tabs 😉 -- depends on tab-tools

I use / contribute to ApolloGraphQL, and several of their sets of repositories have sub packages: apollo-client, apollo-link

Strictly speaking, they don't actually need yarn workspaces / lerna, as each sub-package & examples can be built/tested/integrated on it's own. They just happen to live in common repos, and that make extracting shared code and simultaneous iteration easier.

@fbartho
Copy link
Author

fbartho commented Mar 21, 2018

As I'm sure you know, you could even maintain the react-native-tabs module as an accelerator package that imports & re-exposes all the tab styles. (I've lately been pondering the apollo-boost and many other packages that do that and what it really brings to the table).

@satya164
Copy link
Member

Multiple packages are too much work that I'd like to avoid. Also all the navigators here share a lot of code, so it would mean another package for the shared code. The main issue here is that metro doesn't support optional dependencies, so there's not much we can do here to make vector icons optional.

Regarding workspaces/lerna, they can't be used in React Native due to no symlink support in metro. In addition they make it impossible to install a package from git and make contribution harder.

Though why do you link vector icons at all if you don't need it?

What's an accelerator package?

@fbartho
Copy link
Author

fbartho commented Mar 21, 2018

@satya164 the workspaces/lerna suggestion would be for development on the linked packages -- the deployed packages would not require workspaces/lerna. If you look at the links I provided above, apollo-client/apollo-link both deploy multiple packages, and have workspaces/lerna for contribution.

And yes, the shared implementations would need to be in a shared package (I called it react-navigation-tab-tools above).

We use react-native link and have a bunch of dependencies that need native dependencies, so we'd have to constantly discard the changes from react-native-vector-icons. This would require a frustrating annoying script to prevent linkage of the extra dependency, and seems like a tooling hack.

An accelerator package (my term), is a package that is very light, but has a bunch of important dependencies, and (optionally) contains some glue code that exposes those dependencies with some sane defaults -- it sort of functions as a "quick-start" to work with all the packages it wraps. apollo-boost is an example of an accelerator package. The code in the package is very simple and just sets some defaults, while importing ~10ish dependencies. You don't have to use the accelerator package, and you can "eject" from the accelerator package by including the dependencies yourself, and gluing things together your own way.

@satya164
Copy link
Member

the workspaces/lerna suggestion would be for development on the linked packages

We still need to use Metro for development which doesn't support symlinks. There are other issues like multiple roots etc. It's technically impossible to use it here. Apollo doesn't use Metro, so it's not relevant here.

We use react-native link and have a bunch of dependencies that need native dependencies, so we'd have to constantly discard the changes from react-native-vector-icons

Why do you run react-native link instead of react-native link package-to-link when you add that dependency? Also you'd need to run this command only when you add a new native dependency, which I assume is not often. I agree that it's an inconvenience, but it's not that frustrating.

Anyway, the real solution here is to have support for optional require in metro. I posted on their discord if they have any plans about it. Let's wait for their reply.

Also cc @grabbou is it possible to blacklist some native modules when running react-native link? If not, what do you think about adding this feature?

@fbartho
Copy link
Author

fbartho commented Mar 25, 2018

In answer to your questions @satya164

We have react-native link set in our postinstall script, that way as dependencies change their internal structure, and add libs, everything is constantly linked correctly. react-native link is supposed to be idempotent and do nothing if your dependency is already linked correctly.

We also use react-native-schemes-manager so that we correctly build with Staging/Production variants in addition to the traditional Debug/Release axis, and so as you add a new dependency, it just made sense to have post-install also call link.

@grabbou, I'd also love this feature, as react-native link is permitted to have interactive questions that block script-based execution, so for a dependency I was considering, I would have also had to hack a solution around that.

@satya164 satya164 transferred this issue from react-navigation/tabs Feb 24, 2020
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

3 participants