Skip to content

Commit

Permalink
[fixed] Call all transition hooks on query changes
Browse files Browse the repository at this point in the history
This commit fires transition hooks on all route handlers in the
hierarchy whenever the query changes, not just the leaf route.

This also fixes a regression in remix-run#499 that throws a TypeError when
there are not yet any routes in the router state (see remix-run#623).

Fixes remix-run#623
  • Loading branch information
mjackson committed Dec 17, 2014
1 parent bd71b9c commit 90cd750
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 19 deletions.
26 changes: 21 additions & 5 deletions modules/__tests__/Router-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ var expect = require('expect');
var React = require('react');
var Router = require('../index');
var Route = require('../components/Route');
var RouteHandler = require('../components/RouteHandler');
var TestLocation = require('../locations/TestLocation');
var ScrollToTopBehavior = require('../behaviors/ScrollToTopBehavior');
var getWindowScrollPosition = require('../utils/getWindowScrollPosition');
Expand Down Expand Up @@ -533,8 +534,22 @@ describe('Router', function () {
});
});

it('execute willTransition* callbacks when query changes', function (done) {
var fromCallbackExecuted = false;
it('executes transition hooks when only the query changes', function (done) {
var fromKnifeCalled = false;
var fromSpoonCalled = false;

var Knife = React.createClass({
statics: {
willTransitionFrom: function () {
fromKnifeCalled = true;
}
},

render: function () {
return <RouteHandler/>;
}
});

var Spoon = React.createClass({
statics: {
willTransitionTo: function (transition, params, query) {
Expand All @@ -543,12 +558,13 @@ describe('Router', function () {
}

expect(query['filter']).toEqual('second');
expect(fromCallbackExecuted).toBe(true);
expect(fromKnifeCalled).toBe(true);
expect(fromSpoonCalled).toBe(true);
done();
},

willTransitionFrom: function (transition, element) {
fromCallbackExecuted = true;
fromSpoonCalled = true;
}
},

Expand All @@ -558,7 +574,7 @@ describe('Router', function () {
});

var routes = (
<Route handler={Nested} path='/'>
<Route handler={Knife}>
<Route name="spoon" handler={Spoon}/>
</Route>
);
Expand Down
29 changes: 15 additions & 14 deletions modules/utils/createRouter.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,22 +91,32 @@ function createMatch(route, params) {
return { routes: [ route ], params: params };
}

function hasMatch(routes, route, prevParams, nextParams) {
function hasProperties(object, properties) {
for (var propertyName in properties)
if (properties.hasOwnProperty(propertyName) && object[propertyName] !== properties[propertyName])
return false;

return true;
}

function hasMatch(routes, route, prevParams, nextParams, prevQuery, nextQuery) {
return routes.some(function (r) {
if (r !== route)
return false;

var paramNames = route.paramNames;
var paramName;

// Ensure that all params the route cares about did not change.
for (var i = 0, len = paramNames.length; i < len; ++i) {
paramName = paramNames[i];

if (nextParams[paramName] !== prevParams[paramName])
return false;
}

return true;
// Ensure the query hasn't changed.
return hasProperties(prevQuery, nextQuery) && hasProperties(nextQuery, prevQuery);
});
}

Expand Down Expand Up @@ -335,6 +345,7 @@ function createRouter(options) {

var prevRoutes = state.routes || [];
var prevParams = state.params || {};
var prevQuery = state.query || {};

var nextRoutes = match.routes || [];
var nextParams = match.params || {};
Expand All @@ -343,27 +354,17 @@ function createRouter(options) {
var fromRoutes, toRoutes;
if (prevRoutes.length) {
fromRoutes = prevRoutes.filter(function (route) {
return !hasMatch(nextRoutes, route, prevParams, nextParams);
return !hasMatch(nextRoutes, route, prevParams, nextParams, prevQuery, nextQuery);
});

toRoutes = nextRoutes.filter(function (route) {
return !hasMatch(prevRoutes, route, prevParams, nextParams);
return !hasMatch(prevRoutes, route, prevParams, nextParams, prevQuery, nextQuery);
});
} else {
fromRoutes = [];
toRoutes = nextRoutes;
}

// If routes' hooks arrays are empty, then we transition to current route.
// But path is somehow still get changed.
// That could be only because of route query changes.
// Need to push current route to routes' hooks arrays.
if (!toRoutes.length && !fromRoutes.length) {
var currentRoute = state.routes[state.routes.length-1];
fromRoutes = [currentRoute];
toRoutes = [currentRoute];
}

var transition = new Transition(path, this.replaceWith.bind(this, path));
pendingTransition = transition;

Expand Down

0 comments on commit 90cd750

Please sign in to comment.