artsy / emission

⚠️ Deprecated repo, moved to artsy/eigen ➡️ React Native Components
http://artsy.github.io/blog/2018/04/17/making-a-components-pod/
MIT License
618 stars 78 forks source link

Add a Navigation Library #501

Open orta opened 7 years ago

orta commented 7 years ago

For consignments I need to show a modal, with navigation between VCs inside it. I could push this to Eigen, but realistically this is something that over time we'll want to do more of inside Emission. Especially as a bunch of messaging work will probably need it #495

This is a good nuanced issue on the discussion on the state of the Art for Navigation libs.

Constraints:

Current choices:

NavigatorIOS Official

This is the current de-facto navigation option shipped with RN for iOS people. It's not cross platform, either with web or android. Looks pretty solid for our use cases.

Navigation Docs Official

The docs recommend NavigatoriOS and React Navigation

React Navigation

Cross-platform and could work with web stuff. Looks like the nicest overall, but it doesn't use the native view controllers. This means you get fake UIViewControllers et al.

screen shot 2017-05-03 at 10 59 06

This is an acceptable trade-off for web people coming to native, but not an acceptable trade-off for native devs using JS. Note the "Ello" behind the current screen - that's the last screen, still in memory.

note: This issue has been (experimentally) solved in react-navigation with https://github.com/kmagiera/react-native-screens.

React Router Native

screen shot 2017-05-03 at 11 28 13

Same trade-offs as React Navigation, but would work even better with web as it's got API parity with react-router a popular web library.

React-Native Navigation

Cross-platform, third party, mature and maintained but in the midst of a big rewrite which is never great when you're deciding on a dependency. Rewrite aims for:

Complete API redesign, react-native-controllers deprecation and merge, android and iOS redesign, (almost) complete feature parity, screen lifecycle, static Navigation commands, commands per screenId, complete test coverage

So any code added today will definitely need to change tomorrow. Not optimal.

Native Navigation

A big minus for being built in Swift. Forces any app we use Emission in to use the same toolchain as Airbnb. This lib is also not used in any of their production apps either. Too soon ™️.

Shame, they have the same kind of design constraints as we do, would have been nice to work with.


Why no Swift?

TLDR: RN is bleeding edge, Swift is bleeding edge, keeps our focus on only one place to get burned.

We do have Swift in Eigen, which uses our React Native - that is something that we probably won't be undoing. But other iOS apps that we have (including the host app for these components) are Obj-C only. I don't want to force every app into the Swift rat-race for the needs of a transitive dependency on the app. Longer summary here.


Sumary

And so, it looks like today, I think NavigatorIOS from core is probably the best option. This should be looked at again once React-Native Navigation v2 is at like 2.1, or 2.2. There definitely is a movement towards deprecating the class, but it's been there for a very long time so we'll have some warning.

If we're forced into a corner where we need something better, then React-Native Navigation is probably the next best option for our constraints.

alloy commented 7 years ago

Yeah all non-native options are an absolute no go imo.

Bummer that Native Navigation is done in Swift, I really don’t want any Swift in there. Does NavigatorIOS support callbacks from pushed/modal VCs, like Native Navigation does?

orta commented 7 years ago

Hrm, I'm not sure what you mean by callbacks - you mean the NavigationResult which is returned? Or the url -> view routing?

I'm guessing the answer is no FWIW. I think React-Native Navigation has the same kind of API.

alloy commented 7 years ago

The Promise<NavigationResult> return value. It seems very handy, in retrospect, for e.g. the refine view.

orta commented 7 years ago

Agreed, present's API is particularly good here:

(Promise): A promise that resolves when the presented screen gets dismissed.

Not useful enough to override the Swift drawback, and it doesn't look like there's something directly like this in React Native Navigation.

alloy commented 7 years ago

Oh no, totally, def not adding a Swift dependency.

A quick look at the code reveals that the code is probably too large to do an easy port to objc and I also doubt that @lelandrichardson would accept a PR for that (not technically required of course, but means we need to maintain a dependency that’s larger than a few LOC).

So maybe we should just keep our own lean version that does just what we need and steal some ideas from Native Navigation?

KrauseFx commented 7 years ago

I'm actually very interested in the outcome of this, I had the same question when deciding what to use for the wwdc.family app, and was surprised that there is no good native built-in ones that support both platforms.

alloy commented 7 years ago

@KrauseFx If a Swift dependency is ok for you, I would definitely suggest you checkout Native Navigation. The talk about it from React conf makes it look pretty sweet https://www.youtube.com/watch?v=tWitQoPgs8w

KrauseFx commented 7 years ago

Yeah, I don't really feel like adding 15MB to my binary :trollface:

alloy commented 7 years ago

Right, if you don't have Swift libs in there yet it's an especially tough sell.

maicki commented 7 years ago

I looked into this a while back too and as I had the same problems with all the other solutions (it's especially unfortunate that native navigation is in Swift) so I implemented a pretty rough small homegrown solution in objc: ReactNativeNavigationPlayingAround just to get an idea how this could work. As said it's pretty rough, but the idea was just to get some ideas running without really spending so much time on file separation etc.

There are actually a couple of different ways how you could tackle that, either you can go the view controller way that receives events from the RN navigation components or have a root view that listens for navigation events. I build both in there with examples how to use a ReactNativeNavigationController and how to just use a plain UIViewController that has a specific RCTRootView subclass (ReactNativeNavigation) as root view that listens to navigation events and forwards it to a delegate.

@alloy @orta maybe this is helpful if you decide to build your own lean version.

@KrauseFx By coincident I got an invite today for wwdc.family so I could install the app on my phone. I think it would not be too difficult to build your screens on top of a regular view controller stack, but it also depends how you bootstrapped the RN project, e.g. if you used react-native init or created the app from scratch via Xcode.

orta commented 7 years ago

@maicki - this is definitely enough for us to get started with. I ran through the demo, and it has all the sorts of things we would need today. I'll probably look into adding all these in the next two weeks - so thanks for the option, I may turn it into something 👍

alloy commented 7 years ago

@maicki Nice, thanks!

orta commented 7 years ago

From inside the React-Native Navigation discord:

screen shot 2017-05-08 at 11 03 08

DanielZlotin commented 7 years ago

I would like to step in and say that features like returning a Promise from commands (that will resolve once the transition is complete) is in the roadmap of react-native-navigation.

I'm happy for any and all feedback, let's make navigation great again 😄

tamastimar commented 7 years ago

@orta Hi! Coming from Intro to React Native for an iOS Developer.

Navigation Official

Production worthy and in RN core, but is basically React Navigation below.

That link is pointing to an overview article where there is no component called Navigation.

Is that a thing still available?

orta commented 7 years ago

Ah, I'll re-write that, I think I was going for "this is the recommended pattern, but it's basicall react-navigation"

orta commented 7 years ago

Check-out - https://github.com/APSL/react-native-navigator-wrapper

aksswami commented 6 years ago

@orta Above navigation wrapper is deprecated. So what is the current approach from navigation for emission?

alloy commented 6 years ago

@aksswami Still the same tiny homegrown solution we’ve been using from the start. Search this repo for all files related to ‘switchboard’.

orta commented 6 years ago

https://github.com/maicki/react-native-router-bridge - this can bridge our existing ARSwitchBoard with react-router

brentvatne commented 6 years ago

I don't think this will impact your decision at all and it's not my intention to attempt to, I just wanted to point out for posterity that the original screenshots that show every stack screen in the view hierarchy is an issue that has been (experimentally) solved in react-navigation with https://github.com/kmagiera/react-native-screens.

orta commented 6 years ago

Awesome, I'll edit the original post to include this 👍