Dispatch success or failure only for the last navigateAction

[gpbl]

If more navigateActions are called in sequence, the first actions should never dispatch failure or success since they get "invalidated" by the last action, which started a new navigation.

The change in this PR makes navigateAction to check the URL of the route that the store is expecting to handle: if the action's callback refers to a different url, it does not dispatch NAVIGATION_SUCCESS or NAVIGATION_FAILURE.

See this video showing the problem this PR should solve: when the user clicks multiple times on the navigation bar, the URL may go out of sync with what is actually displayed on the page. The last clicked is the link to "components.html" but the page actually shown is the API for Fluxible. This is particularly visible on slow or irregular connections. I've seen this issue also in https://github.com/gpbl/isomorphic500/issues/44.

[mridgway]

One idea I would like to explore in solving this is using a transaction ID that is dispatched with the route that would allow us to keep track of which NAVIGATE_START a NAVIGATE_SUCCESS is associated with.

In the latest version of Fluxible we introduced a context.rootId that is a generated identifier for a root level executeAction. If this was passed with the payload to the above events, the RouteStore keep track of the ID and could ignore any NAVIGATE_SUCCESS this is not associated with the latest NAVIGATE_START.

I find this preferable than short-circuiting in the action because there may be a preference by the developer to handle these events in some other manner.

[yahoocla]

CLA is valid!

[gpbl]

I agree! However, the payload for the success action is a route, and for the failure, an error object: how should we pass the rootId to their payload?

[mridgway]

Yeah, that's the unfortunate part. It may have to be a breaking change if we want to send the ID.

One thing that we could do in the mean time that gives similar behavior is change this PR to check the latest START url to the SUCCESS url. If SUCCESS doesn't match the latest START then it gets ignored (return here: https://github.com/yahoo/fluxible-router/blob/master/lib/RouteStore.js#L48).

[gpbl]

However I understand now why checking the url doesn't work very well: if the navigation sequence is routeA → routeB → routeA, we would still dispatch two success for routeA, since they have the same url. Or, worse, if one of the actions returns an error, we may dispatch a failure and a success.

[gpbl]

Now i remember why I put the logic in navigateAction and not in the RouteStore: while in _handleNavigateSuccess i can check the route's url, since it comes as payload, I can't do that in _handleNavigateFailure, where i have only the error.

The transaction id seems the way to go, now I'm afraid to uglify the nice API we had so far:

I'd dispatch actions as:

context.dispatch('NAVIGATE_START', { 
     navigationId: context.rootId, 
     payload: payload // naming feels bad here
});
context.dispatch('NAVIGATE_SUCCESS', { 
     navigationId: context.rootId, 
     route: route 
});
context.dispatch('NAVIGATE_FAILURE', { 
     navigationId: context.rootId, 
     error: error 
});

[mridgway]

I think we could get away with adding transactionId to the existing payloads:

context.dispatch('NAVIGATE_START', Object.assign({ transactionId: context.rootId }, payload));

context.dispatch('NAVIGATE_SUCCESS', route.set('transactionId', context.rootId));

error.transactionId = context.rootId; 
context.dispatch('NAVIGATE_FAILURE', error);

It's a little hacky, but preserves backwards compatibility for now.

[mridgway]

Marking this as a breaking change just to keep track of potential changes that we want to make like adding more data to the payload as you mentioned.

[gpbl]

I've applied your suggestions to my PR. To reduce the impact of the change, we can reuse the existing transactionId already passed to the payload.

PS. I'd test this PR where both navigateAction and RouteStore play together, so ideally in the NavLink test, but I think https://github.com/yahoo/fluxible/issues/296 must be closed first.

[mridgway]

I created https://github.com/yahoo/fluxible-router/pull/103 to add transactionId (based on branch that removes immutable). @gpbl want to take a look?

[gpbl]

@mridgway looking great 😍 Thanks a lot!