Adding react-native-modal to eigen

[sweir27]

New Dependency

Name: react-native-modal

URL: https://github.com/react-native-community/react-native-modal

Focus

Wraps the react-native Modal component and enhances it with additional animation and display features. Makes it really easy to do things like animating a modal sheet from the bottom and having the background fade. The fact that it has a simple API and hooks into the RN modal means it's easy to plug in and would be easy to remove if RN adds these features to their own lib.

See example PR here: artsy/eigen#3267

Check List

• Have read over the source code
• Has had a release in the last year, or looks done and stable
• Could you fit this codebase in your head after reading the source?
• Is this the stand-out obvious answer to a particular domain problem?
• Do you expect your team to be the only people who know about this dependency?
• Is this obviously being used in production by the maintainers, and thus battle-tested?
• Does our bundle already include a (transitive) dependency that solves the problem and could we use that instead?
• Do you feel well versed in the domain of this dependency and/or could maintain it if that needs to become an
  option?

Alternatives

• Re-implement functionality ourselves

The library is lightweight so I think we'll save time and effort (in addition to having a community) by using the dependency, but we could certainly build the functionality ourselves.

[ashfurrow]

Awesome, thanks Sarah! The MX team went over the RFC in our Knowledge Share this morning and we agree it makes sense to add this dependency 👍 The PR you added was particularly helpful. Please feel free to resolve at your convenience.

We also talked this RFC process itself. We think it some cases, like this one, the RFC process can introduce extra friction where it’s not needed. However, we felt it’s worth having the process in place to make sure larger dependencies get discussed. As someone who just went through the process, do you have any feedback on it? Was this too onerous? Etc. Thanks again!

[sweir27]

@ashfurrow good question! Here are some thoughts on this case:

• I included some reasoning in my PR that added the dependency, so it felt a little redundant to also create this issue (although the checklist here is more comprehensive).
• I worry in general that the requirements to read through the source code/fit it in ones head could cause some developers to feel uncomfortable about adding dependencies (not a blocker in this case, but I had that thought as I was reading through).
• We also allow for PRs to serve as RFCs, especially for things like adding documentation. Would it be possible to adapt this to a PR workflow (maybe a description template)? That would have the added benefit of colocating the dependency addition with the repo it's introduced to.

[ashfurrow]

I like the idea of treating a PR-as-RFC, especially for smaller dependencies like this one. For a larger dependency, say Redux for example, I wouldn't want to encourage engineers to finish a complete PR's worth of work before adding the PR. I'll look into GitHub's tooling around a description template.

The checklist is more of a list of things to consider before adding the dependency, to encourage developers to take ownership of the code they add to the app bundle. I agree that the requirement to fit the entire source code in one's head is onerous – I never considered it a must-have, so maybe we can reword the RFC template to clarify the purpose of the list a bit.