Skip to content
This repository has been archived by the owner on Feb 25, 2020. It is now read-only.

Lazy initialized MaterialTopTabNavigator routes #9

Merged
merged 5 commits into from
Jul 26, 2018

Conversation

charpeni
Copy link
Member

@charpeni charpeni commented Apr 3, 2018

Routes in MaterialTopTabNavigator are now lazy initialized like in MaterialBottomTabNavigator.

A scene visibility is computed from multiple states and props:

To handle the pan between tabs, we check if you're currently swiping between tabs and the prop lazyOnSwipe is true (default value) or if the tab have been already loaded, we'll check if this tab is a sibling of the focused tab. Then, we'll display the tab if it's a sibling.

With the prop animationEnabled to true, we shouldn't hide a tab before the transition is done. So we're waiting COMPLETE_TRANSITION action to hide it. Also, if the prop sceneAlwaysVisible is true (default value), we won't hide scenes between A and D while transitioning.

If the current tab has not been loaded and must not be visible, we do not render it.

I'll update the docs accordingly to this PR.

tabs-2

@@ -24,8 +24,16 @@ export type InjectedProps = {
screenProps?: any,
};

type State = {
isSwiping: boolean,

This comment was marked as abuse.

@@ -119,6 +135,9 @@ export default function createTabNavigator(TabView: React.ComponentType<*>) {
navigation={navigation}
descriptors={descriptors}
screenProps={screenProps}
onSwipeStart={this._handleSwipeStart}

This comment was marked as abuse.

@@ -17,16 +18,35 @@ type Props = InjectedProps & {
tabBarPosition?: 'top' | 'bottom',
tabBarComponent?: React.ComponentType<*>,
tabBarOptions?: TabBarOptions,
lazyOnSwipe: boolean,

This comment was marked as abuse.

This comment was marked as abuse.

@@ -17,16 +18,35 @@ type Props = InjectedProps & {
tabBarPosition?: 'top' | 'bottom',
tabBarComponent?: React.ComponentType<*>,
tabBarOptions?: TabBarOptions,
lazyOnSwipe: boolean,
sceneAlwaysVisible: boolean,

This comment was marked as abuse.

@@ -17,16 +18,35 @@ type Props = InjectedProps & {
tabBarPosition?: 'top' | 'bottom',
tabBarComponent?: React.ComponentType<*>,
tabBarOptions?: TabBarOptions,
lazyOnSwipe: boolean,
sceneAlwaysVisible: boolean,

This comment was marked as abuse.

@@ -113,18 +133,109 @@ class TabView extends React.PureComponent<Props> {

_renderPanPager = props => <TabViewPagerPan {...props} />;

_renderScene = ({ route, focused }) => {
const { renderScene, animationEnabled, swipeEnabled } = this.props;
componentWillMount() {

This comment was marked as abuse.

}
}

componentWillReceiveProps(nextProps) {

This comment was marked as abuse.

}
}

_onAction = payload => {

This comment was marked as abuse.

transitioningFromIndex:
(payload.lastState && payload.lastState.index) || 0,
});
} else if (payload.action.type === 'Navigation/COMPLETE_TRANSITION') {

This comment was marked as abuse.

This comment was marked as abuse.

@charpeni
Copy link
Member Author

@satya164 I have done your requested changes and upgraded it to match with react-navigation@2.0.1.

@satya164
Copy link
Member

I'll check this over the weekend.

@charpeni
Copy link
Member Author

charpeni commented Jun 1, 2018

I've done some non-scientific benchmarks in a application that have many — probably too much — tabs, and we have noticed a significant improvement with the memory.

Check these videos below:

createMaterialTopTabNavigator from this PR.

createMaterialTopTabNavigator from react-navigation@2.0.4

createTabNavigator (deprecated) from react-navigation@2.0.4

@satya164
Copy link
Member

satya164 commented Jun 1, 2018

I'm sorry I haven't been able to test this. Will definitely do tomorrow. Also it'll be great to have this feature directly in react-native-tab-view instead of here :)

Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After trying this on device, it doesn't work very well with swipes as I suspected. For example, when you are on tab 1, and then you swipe quickly to reach tab 3, tab 3 will stay blank until the animation ends. I updated the example app on master to add an image gallery, so it should be visible after you rebase.

This is a very useful optimization otherwise, so I think landing this, but turning it off by default would be the best choice. Also should avoid extra setStates when the feature is turned off.

Also since we added lazy prop in bottom tabs, we should add it here. We can also add another prop optimizationsEnabled for the clipped thingy and set it to false by default.

const mustBeVisible = this._mustBeVisible(props);

if (!loaded.includes(index) && !mustBeVisible) {
return null;

This comment was marked as abuse.

animationEnabled: true,
};

static getDerivedStateFromProps(nextProps, prevState) {

This comment was marked as abuse.

This comment was marked as abuse.

@charpeni
Copy link
Member Author

charpeni commented Jun 5, 2018

Also it'll be great to have this feature directly in react-native-tab-view instead of here :)

I totally agree I should have done that back in the days. But for now, I just want to land this and maybe improve it and port it to react-native-tab-view after.

This is a very useful optimization otherwise, so I think landing this, but turning it off by default would be the best choice. Also should avoid extra setStates when the feature is turned off.

Also since we added lazy prop in bottom tabs, we should add it here.

Extra setStates are avoided in case lazy is false.

But if we want to go further, these extra setStates are useful to properly lazy load scenes on swipe. If we really want to avoid these extra setStates, we would have to do one of the following:

  • Disable lazy loading if swipe is enabled.
  • Lazy load on focus instead of swipe.

In case swipe is disabled, _handleSwipeStart won't be called.
In case animation is disabled, _handleAnimationEnd and _handleIndexChange won't be called (except on swipe).

We can also add another prop optimizationsEnabled for the clipped thingy and set it to false by default.

Sure, but bottom tabs are already using the clipped thingy (ResourceSavingScene) without optimizationsEnabled prop.

Thoughts?

@satya164
Copy link
Member

satya164 commented Jun 5, 2018

Extra setStates are avoided in case lazy is false.

I meant extra setStates when lazy is turned off. Since we don't need them.

We can also avoid setStates for the resource saving stuff. I did that in bottom navigation. But that's a different thing.

Sure, but bottom tabs are already using the clipped thingy (ResourceSavingScene) without optimizationsEnabled prop.

Yeah, but in bottom tabs it's impossible to notice the optimization. Here we can see the blank page when swiping, so it's important to have it customizable.

Disable lazy loading if swipe is enabled. Lazy load on focus instead of swipe.

I was thinking about heuristics to enable/disable lazy. But then I thought it'll be less confusing to make it explicit. I'll defer to @brentvatne on this.

@charpeni
Copy link
Member Author

charpeni commented Jun 5, 2018

I meant extra setStates when lazy is turned off. Since we don't need them.

Introduced in the last commit. 👍

Thanks for the feedback.

@charpeni
Copy link
Member Author

I was thinking about heuristics to enable/disable lazy. But then I thought it'll be less confusing to make it explicit. I'll defer to @brentvatne on this.

@brentvatne Any thoughts?

@brentvatne
Copy link
Member

let's skip heuristics for now and just make it explicit

@charpeni
Copy link
Member Author

It should be fine like this then 😃

@satya164
Copy link
Member

Can we turn off lazy by default since it doesn't work well in all scenarios?

@charpeni
Copy link
Member Author

Done!

@satya164 satya164 merged commit a4ea2bc into react-navigation:master Jul 26, 2018
@Donhv
Copy link

Donhv commented Jul 29, 2018

@charpeni why i update latest version react-navigation (2.9,3) and lazy in creatMaterialTopTabNavigator not work?

@charpeni
Copy link
Member Author

Because this PR is not included in react-navigation yet.

charpeni added a commit to react-navigation/react-navigation.github.io that referenced this pull request Aug 1, 2018
@charpeni charpeni deleted the tabs-lazy-initialized branch August 1, 2018 19:39
@Donhv
Copy link

Donhv commented Aug 8, 2018

@charpeni how I can use lazy in creatMaterialTopTabNavigator :(

@nebulashine
Copy link

Same question here.

I'm using "react-navigation": "^2.14.2" but found lazy not effective - createMaterialTopTabNavigator is still rendering all screens at once.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants