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

Navigation Experimental TabReducer pre-renders all views #8496

Closed
suedama1756 opened this issue Jun 29, 2016 · 8 comments
Closed

Navigation Experimental TabReducer pre-renders all views #8496

suedama1756 opened this issue Jun 29, 2016 · 8 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@suedama1756
Copy link

I'm using the Navigation Experimental API in react-native version 0.26.3. It seems that the CardStack and related views create every view in advance when used with the TabReducer. In my case I first noticed this because one of my views had a TextInput on it with autofocus and although this view had never been shown the keyboard popped up whenever I started the application. I can over come this by tracking active views in my store (I'm using redux), but this feels wrong as the API is inconsistent depending on use case.

IMO The views should not be created up front, when using the StackReducer I can create views on demand an transitions seem to be just as fluid so I don't belief there is a significant performance issue.

In addition to this I'm concerned that normally in an android application each view would be in its own activity, and the OS would be able to unload pushed views if resources were low, how would this be achieved in react native. On top of that any changes to the underlying store (redux) for state shared across views would cause pushed views to be updated even though they are not on screen.

@jmurzy
Copy link
Contributor

jmurzy commented Jun 30, 2016

If you do not want to retain a specific view, you'd have to remove the route (renamed from scene) that has a reference to it from your navigationState, regardless of how you set your index.

Also, NavigationExperimental has evolved rapidly during the last few releases. So if you run into bugs with v0.26.3, chances are the fix is in a more recent release if not in master. So please upgrade to a more stable release, v0.29-rc, and use NavigationTransitioner before filing issues.

🍺

@suedama1756
Copy link
Author

Thanks for your response however, I think you misunderstand my issue, I've have looked extensively through the code in master for NavigationExperimental and although there has been some simplification, renaming etc. its fundamentally the same underlying principle.

Its not that I want to discard my view, discarding the view could cause its view state to be lost; for example scroll positions, etc. (something that would not happen with native android activities). Also discarding the route before transitioning causes an immediate transition (no animation), so in fact you'd need to update the state twice, once to do the transition, once to remove the unwanted view (also in my version this only works once).

Once you start getting into this territory you find that Navigation experimental does very little for you, swapping and animating views is not particularly tricky. TBH: given the experimental nature of the feature and API I thought you'd be more receptive to the feedback, even if to make note of caveats in the documentation. I'm more than willing and capable of offering support, and PRs in this regard.

Also recommending a RC version as more stable than the officially released 0.28 version seems counter intuitive to me.

I'm currently working on the train with poor connectivity, once I'm in a more suitable location I will update to see if any of the issues are fixed or at least easier to resolve.

@ryankask
Copy link
Contributor

I'm not 100% what behaviour you're expecting but in my app I lazily render each tab's card stack. Is that what you mean when you mentioned keeping track of active views?

@hramos
Copy link
Contributor

hramos commented Jun 30, 2016

Regarding using 0.26.3 vs a newer RC release for stability reasons, it is worth pointing out that NavigationExperimental is, as the name implies, an experimental component. Things are changing fast with NavigationExperimental and if you are looking for a stable component, you should stick with Navigator until the "experimental" monicker is dropped.

@jmurzy
Copy link
Contributor

jmurzy commented Jul 1, 2016

@suedama1756 My bad. Sorry. I see what you mean now.

I'm using the Navigation Experimental API in react-native version 0.26.3. It seems that the CardStack and related views create every view in advance when used with the TabReducer.

I'm not sure how you are using the TabReducer but the bundled reducers are deprecated. NavigationExperimetal that ships with v0.26.3 is also very buggy and NavigationAnimatedView (what CardStack uses internally) is also deprecated. The reason I previously recommended using v0.28-rc is that releases prior to that broke NavigationExperimental in one way or another and included half-baked code. But as far as NavigationExperimental's concerned, I believe both v0.28 and 0.29-rc are as good as v0.28-rc.

In my case I first noticed this because one of my views had a TextInput on it with autofocus and although this view had never been shown the keyboard popped up whenever I started the application. I can over come this by tracking active views in my store (I'm using redux), but this feels wrong as the API is inconsistent depending on use case.

Depending on what StyleInterpolator you use, NavigationExperimental will render your views as follows:

      +------------+
    +-+            |
  +-+ |            |
  | | |            |
  | | |  Focused   |
  | | |   View     |
  | | |            |
  +-+ |            |
    +-+            |
      +------------+

—or—

 * +-------------+-------------+-------------+
 * |             |             |             |
 * |             |             |             |
 * |             |             |             |
 * |    Next     |   Focused   |  Previous   |
 * |    View     |    View     |    View     |
 * |             |             |             |
 * |             |             |             |
 * |             |             |             |
 * +-------------+-------------+-------------+

So if you add a route to your navigationState, it will be rendered immediately and retained until removed. Meaning the views are pushed off screen but they are still a part of the view hierarchy. So if you relaunch the app with a hydrated navigationState, regardless of it being the view in focus or not, autoFocus will trigger the keyboard to pop up. 🤔

As @ryankask suggested, you might have to lazily render your tabs so autoFocus doesn't trigger until the view is re-added to the navigationState—you'd also have to disable the transition animations for tabs by providing your own configureTransition function to NavigationTransitioner. I've a PR that will hopefully make dealing with transitions a bit more flexible.

I'm afraid ^ is probably a workaround though (since it doesn't allow you to persist navigationState as a whole), and the issue may need to be addressed in the autoFocus code.

@ericvicenti and @hedgerwang might have better answers for you. As @hramos pointed out above, NavigationExperimental is, well, experimental and is a pretty low level API.

🍺

@suedama1756
Copy link
Author

Thanks guys, I really do appreciate the efforts you've put into the answers @jmurzy . As I said, I'm not really stuck as it was fairly obvious from the code how things worked and how to work around most of these issues. I'm sure others will find this information useful though.

I did notice the built in reducers where removed in master (which I think is a good thing); and I should have upgraded (I hadn't spotted that react-native init fixed the version in package.json) so had assumed I was up to date as my project was created recently (my bad).

I think there is still some misunderstanding about the purpose of my post, I'm not complaining about Navigation Experimental (or the fact its 'Experimental'), I attempting to get involved in ensuring its as good as it can be before it stops being 'Experimental' to ensure a broader range of use cases has been considered.

My point is about resource management, react native by default only creates a single native Android Activity, in most cases you DO want to keep your pushed views as the user will want to return to that views state (e.g. list view scroll position, etc.), some of this state can be re-hydrated from your store, some cannot. In Android background activities will be unloaded if system resources are low, but their view state is persisted. There seems no mechanism for this in react native in terms of unloading a view and rehydration with scroll positions. IMO this could lead to resource issues in large applications, or an inconsistent user experience as you unload views.

I have seen it's possible to create additional native activities, is this not something that should be considered for any built in navigation to ensure efficient resource utilisation?

Please can I ask that if you response you respond by explaining either why additional views in the render tree our not a resource issue, how I can rehydrate an unloaded view exactly, or if there are any future plans to use underlying platform capabilities for activity management (if not I will close the issue and do my own investigation).

@ide
Copy link
Contributor

ide commented Jul 1, 2016

I have seen it's possible to create additional native activities, is this not something that should be considered for any built in navigation to ensure efficient resource utilisation?

To chime in on this point -- the underlying Activity and UINavigationController paradigms on each platform are different enough that it ends up being a lot better to write the navigation system in pure JS (as surprising as that sounds maybe?).

The way we've (we = my team, not NavigationExperimental) address memory management is in a couple ways. One way is to profile and look at real-world use cases to see if it's even a problem, and it hasn't been too bad in practice. Android devices have 2~8GB of RAM these days. iPhones come with 2GB of RAM and are pretty efficient.

Another thing we've done is to build a library on top of NavigationExperimental (https://github.com/exponentjs/ex-navigation) that has the notion of a focused navigator. This provides an opportunity for us to write code to hide views / release data structures / do whatever is appropriate for our needs when scenes lose and regain focus. We prefer this greatly over the complicated Android life cycle (Square wrote a good post: https://corner.squareup.com/2014/10/advocating-against-android-fragments.html) and the full UINavigationController API which also has a high level of complexity. And we fully control how to do rehydration -- for us that means storing all state we care about in Redux, for someone else it could be another mechanism. In a lot of cases you don't need to tear down your entire scene -- it might be sufficient just to hide big images and GIFs.

The last thought I want to leave here is that I believe it's really important that NavigationExperimental is not too prescriptive and allows us to accommodate needs we can't anticipate. Having no abstraction is better than the wrong abstraction and we're still in the early days of Navigation and React Native figuring out those abstractions to meet real needs of products like facebook.

@mkonicek
Copy link
Contributor

Hi there! This issue is being closed because it has been inactive for a while.

But don't worry, it will live on with ProductPains! Check out its new home: https://productpains.com/post/react-native/navigation-experimental-tabreducer-pre-renders-all-views-

Product Pains has been very useful in highlighting the top bugs and feature requests:
https://productpains.com/product/react-native?tab=top

Also, if this issue is a bug, please consider sending a pull request with a fix.

@facebook facebook locked as resolved and limited conversation to collaborators Jul 19, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

7 participants