From 90cd7502caa157068849da85e0eec972ec175a19 Mon Sep 17 00:00:00 2001
From: Michael Jackson <mjijackson@gmail.com>
Date: Wed, 17 Dec 2014 11:24:36 -0800
Subject: [PATCH] [fixed] Call all transition hooks on query changes

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 #499 that throws a TypeError when
there are not yet any routes in the router state (see #623).

Fixes #623
---
 modules/__tests__/Router-test.js | 26 +++++++++++++++++++++-----
 modules/utils/createRouter.js    | 29 +++++++++++++++--------------
 2 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/modules/__tests__/Router-test.js b/modules/__tests__/Router-test.js
index 7b9bad4b5e..730d30008a 100644
--- a/modules/__tests__/Router-test.js
+++ b/modules/__tests__/Router-test.js
@@ -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');
@@ -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) {
@@ -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;
           }
         },
 
@@ -558,7 +574,7 @@ describe('Router', function () {
       });
 
       var routes = (
-        <Route handler={Nested} path='/'>
+        <Route handler={Knife}>
           <Route name="spoon" handler={Spoon}/>
         </Route>
       );
diff --git a/modules/utils/createRouter.js b/modules/utils/createRouter.js
index 42abb9dab6..2109d21fa6 100644
--- a/modules/utils/createRouter.js
+++ b/modules/utils/createRouter.js
@@ -91,7 +91,15 @@ 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;
@@ -99,6 +107,7 @@ function hasMatch(routes, route, prevParams, nextParams) {
     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];
 
@@ -106,7 +115,8 @@ function hasMatch(routes, route, prevParams, nextParams) {
         return false;
     }
 
-    return true;
+    // Ensure the query hasn't changed.
+    return hasProperties(prevQuery, nextQuery) && hasProperties(nextQuery, prevQuery);
   });
 }
 
@@ -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 || {};
@@ -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;