NavigationExperimental behavior is buggy when replacing stack entries

[kushal]

Or maybe I'm misunderstanding what's supposed to happen.

Let's say my initial navigation state is
{ key: 'root', index: 0, children: [ { key: 'home' } ] }

and then I replace it with
{ key: 'root', index: 0, children: [ { key: 'settings' } ] }

As far as I can tell, NavigationScenesReducer, via NavigationAnimatedView, will keep rendering both settings and home, and since they both have the same index, compareScenes ends up sorting by alpha. Should isStale be part of compareScenes?

(Also, to be honest, I don't totally understand why stale scenes are part of the result returned by NavigationScenesReducer, it'd be great to have some more comments in the code to explain why it works this way.)

[kushal]

cc @ericvicenti

[ryankask]

I experienced the same problem yesterday and searched around and found a relevant few comments/issues:

• renderOverlay receives wrong scene after replace ericvicenti/navigation-rfc#73 - renderOverlay receives wrong scene after replace
• Support for "replace" navigation action? ericvicenti/navigation-rfc#50 -Discussion with author
• Type 'replace' not replacing view aksonov/react-native-router-flux#474
• 0.25 Support aksonov/react-native-router-flux#636 - suggests skipping 0.25 and waiting for 0.26
• aksonov/react-native-router-flux@e828fb0 - this PR addresses this issue and I implemented it in my project (it's a hack).

There are probably other GitHub issues around describing the same problem.

[dbasedow]

I reported ericvicenti/navigation-rfc#73. The stale scenes are kept around during animation. They are removed in NavigationAnimatedView._onProgressChange after the transition completed.

The issue with the same numeric index is very easy to fix (as mentioned in my other issue). After thinking about it some more, I think the best fix would be to change NavigationScenesReducer not to produce a stale scene if there already is a fresh scene at the same index with a different key.

[mmazzarolo]

I'm a newbie in NavigationExperimental and I'd like to know something more about the magic behind this:

(Also, to be honest, I don't totally understand why stale scenes are part of the result returned by NavigationScenesReducer, it'd be great to have some more comments in the code to explain why it works this way.)

I also don't get the reason behind having the scenes saved in the internal state of NavigationAnimatedView, which is causing some confusion when replacing the stack entries with a custom reducer...

[ericvicenti]

Hey folks, sorry to hear about the troubles! Also cc @hedgerwang

(Also, to be honest, I don't totally understand why stale scenes are part of the result returned by NavigationScenesReducer, it'd be great to have some more comments in the code to explain why it works this way.)
I also don't get the reason behind having the scenes saved in the internal state of NavigationAnimatedView, which is causing some confusion when replacing the stack entries with a custom reducer...

It is done in this way so that the scene stays around for the duration of the animation as the removed scene exits the screen. We keep the internal state in the AnimatedView because it is usually a bad idea to maintain the animation state in your app state.

That said, it seems like we have a bug in the scenes reducer while doing a replace. If somebody wants to debug this, write a failing test case, or add a failing UIExplorer 'replace' example, we would greatly appreciate it!

[kushal]

Ah, that's what I suspected (about keeping stale scenes for animation). I suspect one issue is around the animation being tied to index, which in this case would be from to 0 to 0, so the stale scene is never removed. I saw you assigned the bug to yourself - do you still need someone to poke around at this?

[ericvicenti]

Yeah, that's the problem. I was mistaken earlier and thought this was a problem with the scenes reducer. But you're right that this is more of a fundamental design problem. Hedger and I will work on a solution that decouples the transition from the index.

[mmazzarolo]

Hey @ericvicenti, any news regarding this issue? Thank you! 🌊

[jmurzy]

@mmazzarolo I believe @ericvicenti and @hedgerwang are working on a new version of NavigationAnimatedView called NavigationTransitionerthat's been redesigned to address the issues mentioned here. That being said, NavigationTransitioner comes with its own set of issues that need to be addressed before it can replace NavigationAnimatedView.

See jmurzy/react-router-native/issues/3

🍺

[mmazzarolo]

@jmurzy Thank you man!
In the meantime does anybody know a way to reset the stack in the current version of Navigation Experimental (@dbasedow?) or at least a way to unmount the components after a replace so that it can be re-rendered? (simply using navigationStateUtils.reset doesn't remove stale scenes)

[fdecampredon]

I created a little MonkeyPatch for my own purpose of NavigationAnimatedView that allow stack to be replaced, use it at your own risk:

// scenesReducer.js


const invariant = require('fbjs/lib/invariant');
const SCENE_KEY_PREFIX = 'scene_';

/**
 * Helper function to compare route keys (e.g. "9", "11").
 */
function compareKey(one: string, two: string): number {
    const delta = one.length - two.length;
    if (delta > 0) {
        return 1;
    }
    if (delta < 0) {
        return -1;
    }
    return one > two ? 1 : -1;
}

/**
 * Helper function to sort scenes based on their index and view key.
 */
function compareScenes(one, two) {
    if (one.index > two.index) {
        return 1;
    }
    if (one.index < two.index) {
        return -1;
    }

    return compareKey(one.key, two.key);
}

function areScenesShallowEqual(one, two): boolean {
    return (
        one.key === two.key &&
        one.index === two.index &&
        one.isStale === two.isStale &&
        one.navigationState === two.navigationState &&
        one.navigationState.key === two.navigationState.key
    );
}

export default function NavigationScenesReducer(scenes, nextState, prevState) {
    const prevScenes = new Map();
    const freshScenes = new Map();
    const staleScenes = new Map();

    // Populate stale scenes from previous scenes marked as stale.
    scenes.forEach(scene => {
        if (scene.index !== -1) {
            const { key } = scene;
            if (scene.isStale) {
                staleScenes.set(key, scene);
            }
            prevScenes.set(key, scene);
        }
    });

    const nextKeys = new Set();
    nextState.children.forEach((navigationState, index) => {
        const key = SCENE_KEY_PREFIX + navigationState.key;
        const scene = {
            index,
            isStale: false,
            key,
            navigationState,
        };
        invariant(
            !nextKeys.has(key),
            `navigationState.children[${index}].key "${key}" conflicts with` +
            'another child!'
        );
        nextKeys.add(key);

        if (staleScenes.has(key)) {
            // A previously `stale` scene is now part of the nextState, so we
            // revive it by removing it from the stale scene map.
            staleScenes.delete(key);
        }
        freshScenes.set(key, scene);
    });

    if (prevState) {
        // Look at the previous children and classify any removed scenes as `stale`.
        prevState.children.forEach((navigationState, index) => {
            const key = SCENE_KEY_PREFIX + navigationState.key;
            if (freshScenes.has(key)) {
                return;
            }
            staleScenes.set(key, {
                index,
                isStale: true,
                key,
                navigationState,
            });
        });
    }

    const nextScenes = [];

    const mergeScene = (nextScene => {
        const { key } = nextScene;
        const prevScene = prevScenes.has(key) ? prevScenes.get(key) : null;
        if (prevScene && areScenesShallowEqual(prevScene, nextScene)) {
            // Reuse `prevScene` as `scene` so view can avoid unnecessary re-render.
            // This assumes that the scene's navigation state is immutable.
            nextScenes.push(prevScene);
        } else {
            nextScenes.push(nextScene);
        }
    });

    const freshScenesArray = [...freshScenes.values()];
    staleScenes.forEach(scene => {
        if (scene.index === 0 && freshScenesArray.find(({ index }) => index === scene.index)) {
            scene.index = -1;
        }
        mergeScene(scene);
    });

    freshScenesArray.forEach(mergeScene);


    return nextScenes.sort(compareScenes);
}

// NavigationAnimatedView.js
/**
 * Copyright (c) 2015-present, Facebook, Inc.
 * All rights reserved.
 *
 * This source code is licensed under the BSD-style license found in the
 * LICENSE file in the root directory of this source tree. An additional grant
 * of patent rights can be found in the PATENTS file in the same directory.
 *
 */
/* eslint-disable no-underscore-dangle */

import React, { PropTypes } from 'react';
import { Animated, NavigationExperimental, StyleSheet, View } from 'react-native';
import scenesReducer from './scenesReducer';

const {
    Container: NavigationContainer,
    PropTypes: NavigationPropTypes,
} = NavigationExperimental;


function applyDefaultAnimation(position, navigationState, prevNavigationState) {
    if (navigationState.index === prevNavigationState.index) {
        position.setValue(-1);
    }
    Animated.spring(
        position,
        {
            bounciness: 0,
            toValue: navigationState.index,
        }
    ).start();
}

class NavigationAnimatedView extends React.Component {

    static propTypes = {
        applyAnimation: PropTypes.func,
        navigationState: NavigationPropTypes.navigationState.isRequired,
        onNavigate: PropTypes.func.isRequired,
        renderOverlay: PropTypes.func,
        renderScene: PropTypes.func.isRequired,
        style: View.propTypes.style,
    };

    static defaultProps = {
        applyAnimation: applyDefaultAnimation,
    };

    state = {
        layout: {
            height: new Animated.Value(0),
            initHeight: 0,
            initWidth: 0,
            isMeasured: false,
            width: new Animated.Value(0),
        },
        position: new Animated.Value(this.props.navigationState.index),
        scenes: scenesReducer([], this.props.navigationState),
    }

    componentDidMount() {
        this._positionListener = this.state.position.addListener(this._onProgressChange);
    }

    componentWillReceiveProps(nextProps) {
        if (nextProps.navigationState !== this.props.navigationState) {
            this.setState({
                scenes: scenesReducer(
                    this.state.scenes,
                    nextProps.navigationState,
                    this.props.navigationState
                ),
            });
        }
    }

    componentDidUpdate(lastProps) {
        const { index: lastIndex, children: lastChildren } = lastProps.navigationState;
        const { index, children } = this.props.navigationState;
        if (
            lastIndex !== index || (
                index === 0 && 
                children && lastChildren && 
                children.length && lastChildren.length &&
                children[0].key !== lastChildren[0].key
            )
        ) {
            this.props.applyAnimation(
                this.state.position,
                this.props.navigationState,
                lastProps.navigationState
            );
        }
    }

    componentWillUnmount() {
        this.state.position.removeListener(this._positionListener);
    }

    _onProgressChange = (data: Object) => {
        const delta = Math.abs(data.value - this.props.navigationState.index);
        if (delta > Number.EPSILON) {
            return;
        }

        const scenes = this.state.scenes.filter(scene => !scene.isStale);

        if (scenes.length !== this.state.scenes.length) {
            this.setState({ scenes });
        }
    }

    _onLayout = (event) => {
        const { height, width } = event.nativeEvent.layout;

        const layout = {
            ...this.state.layout,
            initHeight: height,
            initWidth: width,
            isMeasured: true,
        };

        layout.height.setValue(height);
        layout.width.setValue(width);

        this.setState({ layout });
    }

    _renderScenes() {
        return this.state.scenes.map(this._renderScene, this);
    }

    _renderScene(scene) {
        const {
            navigationState,
            onNavigate,
            renderScene,
        } = this.props;

        const {
            position,
            scenes,
        } = this.state;

        return renderScene({
            layout: this.state.layout,
            navigationState,
            onNavigate,
            position,
            scene,
            scenes,
        });
    }

    _renderOverlay() {
        if (this.props.renderOverlay) {
            const {
                navigationState,
                onNavigate,
                renderOverlay,
            } = this.props;

            const {
                position,
                scenes,
            } = this.state;

            return renderOverlay({
                layout: this.state.layout,
                navigationState,
                onNavigate,
                position,
                scene: scenes[navigationState.index],
                scenes,
            });
        }
        return null;
    }


    render() {
        const overlay = this._renderOverlay();
        const scenes = this._renderScenes();
        return (
            <View
              onLayout={this._onLayout}
              style={this.props.style}>
                <View style={styles.scenes} key="scenes">
                    {scenes}
                </View>
                {overlay}
            </View>
        );
    }
}

const styles = StyleSheet.create({
    scenes: {
        flex: 1,
    },
});

export default NavigationContainer.create(NavigationAnimatedView);

[mmazzarolo]

@fdecampredon I admit I didn't even investigate on what you're patching in those two components but they works fine for me, thank you 🍰

[mmazzarolo]

Hey there, I just wanted to let you know that @fdecampredon solution works on react-native@0.27.1 too, just remember to change:

export default NavigationContainer.create(NavigationAnimatedView);

to:

export default NavigationAnimatedView;

Still hoping for a more serious solution though.

[jmurzy]

@mmazzarolo NavigationAnimatedView is deprecated. NavigationTransitioner in v0.28.0-rc.0 should address most of these issues. Can you upgrade and let us know if it resolves it for you?

🍺

[mmazzarolo]

@ericvicenti @jmurzy
Sorry man, I've been really busy lately.
Sadly I'm still having the issue on RN 28...
I'm not doing anything too fancy, I just have a CardStack wrapped in Drawer:

<Drawer
  currentRoute={currentRoute}
  onNavigate={this._handleAction}
>
  <CardStack
    navigationState={navState}
    onNavigate={this._handleAction}
    renderScene={this._renderScene}
  />
</Drawer>

The drawer has some button that when pressed should reset the stack and bring the user to the specific route.
The reducer reset action is the following (I changed it like 999 times):

case 'NAV_RESET': {
  const { route } = action
  return NavigationStateUtils.reset(state, [route], 0)
}

Unfortunately resetting the scene stack this way doesn't work: the new route is not rendered.
The reducer works correctly but I can clearly see that the in NavigationExperimental's scenes there are still the old routes (with isStale: true).

This setup worked perfectly with @fdecampredon solution.
Do you have any suggestions?

[jmurzy]

@mmazzarolo Sorry about this. I think your issue may be related to c57bac4#commitcomment-17977007. Both @hedgerwang and @ericvicenti have been working hard in fixing things up over last few days. See c57bac4, b4d15d3.

Unfortunately though, these patches did not make it into 0.29-rc. I hope to get those cherry picked into 0.29, #8333.

If you are still having issues after applying above patches and have a repo that reproduces the issue, I'm happy to take a look.

🍺

[mmazzarolo]

Thank you and no worries @jmurzy , I'll try to update as soon as the commits get cherry picked 🏄

[finalquest]

Hi.
I'm facing the same situation as @mmazzarolo
The reset scene does not render the new routes.
I've applied the patchs suggested by @jmurzy, but the behaviour is still the same.

As @mmazzarolo states, the solution provided by @fdecampredon worked.

Regards.

[yonahforst]

I'm trying to apply @fdecampredon's solution on RN-0.28.0

Do I just replace the entire contents of NavigationAnimatedView.js with the code above? they are pretty different so it's hard to figure out what changed and how to apply it correctly.

Also, I don't have an existing scenesReducer.js should i create it?

[mmazzarolo]

Hey @yonahforst, I suggest to copy NavigationAnimatedView.js and scenesReducer.js in your own project and then to use the copied NavigationAnimatedView instead of the RN one.

scenesReducer.js is just a dependency of NavigationAnimatedView.js as you can see from row 15:

import scenesReducer from './scenesReducer';

you don't need to use it directly

[yonahforst]

Hey @mmazzarolo, and thanks for the quick reply!

Thing is, I'm not actually using NavigaitionAnimatedView anywhere. I have a similar setup to what you described above, just a CardStack wrapped in a drawer.

[mmazzarolo]

My bad, you're using RN 0.28, I thought you were using RN 0.27 🙈
I don't know if NavigationAnimatedView still works in 0.28 (I guess not since it was deprecated on RN 0.27).
If you're interested in RN 0.27 I used the patched NavigationAnimatedView this way (an I'm still stucked in this version since the stack replacing doesn't work):

    return (
      <Drawer
        currentRoute={currentRoute}
        isOpen={isDrawerOpen}
        isEnabled={isDrawerEnabled}
        onNavigate={this._handleAction}
        navigationActions={this.navigationActions}
      >
        <NavigationAnimatedView
          navigationState={navState}
          onNavigate={this._handleAction}
          renderOverlay={this._renderToolbar}
          style={styles.container}
          renderScene={props => (
            <Card
              {...props}
              panHandlers={null}
              renderScene={this._renderScene}
              key={`card_${props.scene.navigationState.key}`}
            />
          )}
        />
      </Drawer>
    )

You can skip the various custom props and the <Card> component (I used it for only disabling the panHandler).

[yonahforst]

hmmm. didn't work. There were a bunch of breaking changes in 0.28.0 (e.g. navigationState.children was renamed to navigationState.routes). I tried adjusting correcting them but then i ran into some other errors, I'm assuming related to the breaking changes in 0.28.0.

I'm going to see what I can find about implementing a CardStack using the NavigationTransitioner since the AnimatedView deprecated (as you mentioned)

[mmazzarolo]

@jmurzy @hedgerwang @ericvicenti
I sadly confirm what @finalquest said, replacing the stack is still not working on 0.29.0-rc.2 😿

Don't take it as a critic (I know that working on an entire new navigation system can be incredibly hard... I would even like to help but I don't have enough knowledge of it) but: do you have any idea of what can be the source of this bug and/or if can be fixed in a short time?
I'm asking because this bug has been haunting me since April and I need to decide if I should switch to another Navigator (the old one or something like react-native-router-flux) or if I should just wait a bit more (I must be production ready in two months).

And again, thank you for you work guys 👊

[yonahforst]

@mmazzarolo
I was finally able to get mine working on 0.28.0 by replacing NavigationAnimatedView with NavigationTransitioner, basically copying this example: https://github.com/jlyman/RN-NavigationExperimental-Redux-Example/blob/master/app/containers/AppContainer.js

What's your setup? are you still using the solution by @fdecampredon? Here's my navigator component, if it's any help: https://gist.github.com/yonahforst/ef490002d2aed3b412979669347b04b4

[mmazzarolo]

@yonahforst thanks!
Replacing/resetting with NavigationTransitioner does work in 0.28.0 but it does not work with NavigationCardStack (also, NavigationCardStack won't work even on 0.29.0-rc2).

This does not work with stack resetting:

<Drawer
  currentRoute={currentRoute}
  isOpen={navState.isDrawerOpen}
  isEnabled={isDrawerEnabled}
  onNavigate={this._handleAction}
  navigationActions={this.navigationActions}
>
  <NavigationCardStack
    navigationState={navState}
    onNavigate={this._handleAction}
    renderScene={this._renderScene}
    renderOverlay={this._renderToolbar}
    style={styles.container}
  />
</Drawer>

This does work with stack resetting:

<Drawer
  currentRoute={currentRoute}
  isOpen={navState.isDrawerOpen}
  isEnabled={isDrawerEnabled}
  onNavigate={this._handleAction}
  navigationActions={this.navigationActions}
>
  <NavigationTransitioner
    navigationState={navState}
    onNavigate={this._handleAction}
    renderOverlay={this._renderToolbar}
    style={styles.container}
    renderScene={props =>
      <NavigationCard
        {...props}
        panHandlers={null}
        renderScene={this._renderScene}
        key={props.scene.route.key}
      />
    }
  />
</Drawer>

Example of action that resets the stack:

case 'NAV_RESET': {
  const { route } = action
  return NavigationStateUtils.reset(state, [route], 0)
}

Also please keep in mind that the above solution won't work out of the box in the future release of RN 0.29.0: it will need some kind of tweaks (e.g.: renderScene will be renamed to render etc.) but keep an eye on @jlyman example, he's constantly updating it (thanks 👍 ).

(@finalquest you might be interested)

[yonahforst]

@mmazzarolo - yeah, I was also using the NavigationCardStack component and replaced it with NavigationTransitioner. As far as I can tell, I'm not missing anything and everything still pretty much works the same.

and yes, thank you @jlyman!

[jmurzy]

@mmazzarolo @yonahforst NavigationCardStack in v0.29.0-rc2 is still using NavigationAnimatedView— deprecated. This is fixed in master. If NavigationTransitioner fixes your issue, then to fix NavigationCardStack, you would have to cherry pick this (and most probably apply to it this patch that cleans it up further) until a new release is cut.

🍺

[mmazzarolo]

@jmurzy
Replacing works in NavigationCardStack on 0.30.0-rc.0! 🆒

I wanted to report two other "issues" though (also, can you point me to a better place for discussing them?)

1. CardStack
The style applied to NavigationCardStack is now propagated to renderOverlay (it didn't work this way on versions < 0.27.0).
For example using the style { flex: 1, paddingTop: 64 } on NavigationCardStack now applies the padding to the navigation bar too (with the navigation bar being rendered by renderOverlay).
Not a real issue though, just a breaking visual change.

2. Card
panHandlers={null} doesn't disable "swipe to back gesture" anymore (it did on 0.27.0).

And thank you for all you've done and the support 👍

[Chris-Petty]

@mmazzarolo replacing does indeed work on 0.30.0-rc.0, though the animation isn't functioning correctly for me. Seems to just spontaneously change. The title in the nav bar for example, you can see the new title before the old title simply disappears.

[mmazzarolo]

@jmurzy still same situation on React-Native 0.30.0...
Is the stuff above considered the standard now?

[Chris-Petty]

Just going to confirm that my above comment is still accurate in the 0.30 release.

[ronaldheft]

I'm using 0.30, and I've found the prop scene will not be set in render under the following situation:

• Start at index 0
• Push scene to index 1
• Replace scene at index 1 (no animation)
• Pop scene (so back at index 0)
• NavigationCardStack will not pass the prop scene to its render methods

[ng-marcus]

Same issue here with 0.30 - the scene prop is missing after a reset, but seems to be working in 0.31-rc0

[joypatel04]

Can anyone please confirm that is it working on 0.31? @ng-marcus @mmazzarolo

[ng-marcus]

Working fine for me in 0.31 at the moment

[benhughes]

I had the same issue and at seems to be working as expected now in 0.31

[satya164]

Closing this issue as it seems to be working fine now.