Skip to content
This repository has been archived by the owner on Oct 26, 2018. It is now read-only.

setRouteLeaveHook double called #160

Closed
ghost opened this issue Jan 4, 2016 · 5 comments
Closed

setRouteLeaveHook double called #160

ghost opened this issue Jan 4, 2016 · 5 comments

Comments

@ghost
Copy link

ghost commented Jan 4, 2016

I'm having this issue when using redux simple router with setRouteLeaveHook on the context my routerWillLeave callback executes twice.

class MyComponent extends React.Component {
    constructor(props, context) {
        super(props, context);

        this.routerWillLeaveCallCount = 0;
    }
    componentDidMount() {
        this.context.router.setRouteLeaveHook(this.props.route, this.routerWillLeave.bind(this));
    }
    routerWillLeave(route) {
        console.log(route.action);
        if (this.hasUnsavedData() && this.routerWillLeaveEvenCallCount()) {
            return 'You have an unsaved data, are you sure you want to leave?';
        }
    }
    routerWillLeaveEvenCallCount() {
        return this.routerWillLeaveCallCount++ % 2 === 0;
    }

I see PUSH PUSH in my console log.
I cloned this repo https://github.com/freeqaz/redux-simple-router-example.git and upgraded it to react-router 2.0.0-rc4 and redux-simple-router 1.0.2 and confirmed it had the same issue.

When I run the example https://github.com/rackt/react-router/tree/master/examples/transitions it only fires once.

@gustavnikolaj
Copy link

I see the same behaviour on my onEnter hooks while using react-router, react-redux, redux and redux-simple-router. Called twice for every component with onEnter with the same action and history key.

@gustavnikolaj
Copy link

Seems that this might be fixed by #141. Haven't been able to test it out unfortunately.

@gustavnikolaj
Copy link

My problem is fixed when linking against 6760e40 :-) 👍

@Ephem
Copy link

Ephem commented Jan 8, 2016

In current versions (1.0.2) the code for updating the store when history has updated looks like this:

} else if(!locationsAreEqual(getRouterState(), route)) {
    // The above check avoids dispatching an action if the store is
    // already up-to-date
    const method = location.action === 'REPLACE' ? replacePath : pushPath
    store.dispatch(method(route.path, route.state, { avoidRouterUpdate: true }))
}

Notice that lastRoute is not updated. avoidRouterUpdate true simply bumps the changeId by one. The store will then trigger into this part:

    // Only trigger history update if this is a new change or the
    // location has changed.
    if(lastRoute.changeId !== routing.changeId ||
       !locationsAreEqual(lastRoute, routing)) {

ChangeId will be the same so the assumption (from avoidRouterUpdate: true) is that history will not update, but since lastRoute has not been updated locationsAreEqual will return false and the history will update a second time, even though it was a history update that triggered the whole thing.

I actually noticed this error since this will always overwrite location.action with either REPLACE or PUSH, which means POP will get lost along the way.

Master seems to fix all this and I really like the changes, I look forward to the next version! :)

@jlongster
Copy link
Member

It's coming soon :) been busy with work but I'll see about releasing it today or tomorrow.

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

No branches or pull requests

3 participants