Merge pull request #242 from nathanhammond/patch-1

Prevent context matching if the oldContext is null

lib/router/handler-info.js

@@ -179,12 +179,19 @@ export default class HandlerInfo {
       payload.params[this.name] = params;
     }
 
-    return this.factory('resolved', {
-      context: resolvedContext,
+    var resolution = {
       name: this.name,
       handler: this.handler,
       params: params,
-    });
+    };
+
+    // Don't set a context on the resolution unless we actually have one.
+    var contextsMatch = resolvedContext === this.context;
+    if ('context' in this || !contextsMatch) {
+      resolution.context = resolvedContext;
+    }
+
+    return this.factory('resolved', resolution);
   }
 
   shouldSupercede(other) {
@@ -201,7 +208,7 @@ export default class HandlerInfo {
     var contextsMatch = other.context === this.context;
     return (
       other.name !== this.name ||
-      (this.hasOwnProperty('context') && !contextsMatch) ||
+      ('context' in this && !contextsMatch) ||
       (this.hasOwnProperty('params') && !paramsMatch(this.params, other.params))
     );
   }
@@ -237,22 +244,6 @@ export default class HandlerInfo {
   }
 }
 
-// this is bonkers, we require that `context` be set on on the
-// HandlerInfo prototype to null because the checks in
-// `NamedTransitionIntent.prototype.applyToHandlers` here
-// https://github.com/tildeio/router.js/blob/v1.2.8/lib/router/transition-intent/named-transition-intent.js#L76-L81
-// check of `oldHandlerInfo.context === newHandlerInfo.context` and assumes
-// that the params _must_ match also in that case.
-//
-// The only reason `oldHandlerInfo.context` and `newHandlerInfo.context` did not
-// match in prior versions is because if the context isn't set yet (on newHandlerInfo)
-// is because it inherits the `null` from the prototype vs `undefined` (on
-// the oldHandlerInfo).
-//
-// A future refactoring should remove that conditional, and fix the hand full of
-// failing tests.
-HandlerInfo.prototype.context = null;
-
 function paramsMatch(a, b) {
   if (!a ^ !b) {
     // Only one is null.

lib/router/transition-intent/named-transition-intent.js

@@ -114,7 +114,11 @@ export default class NamedTransitionIntent extends TransitionIntent {
           newHandlerInfo.context
         );
         var oldContext = oldHandlerInfo && oldHandlerInfo.context;
-        if (result.names.length > 0 && newHandlerInfo.context === oldContext) {
+        if (
+          result.names.length > 0 &&
+          'context' in oldHandlerInfo &&
+          newHandlerInfo.context === oldContext
+        ) {
           // If contexts match in isActive test, assume params also match.
           // This allows for flexibility in not requiring that every last
           // handler provide a `serialize` method

tests/router_test.js

@@ -1549,6 +1549,48 @@ scenarios.forEach(function(scenario) {
     transitionTo(router, 'adminPost', { id: 75 });
   });
 
+  test('check for mid-transition correctness', function(assert) {
+    var posts = {
+      1: { id: 1 },
+      2: { id: 2 },
+      3: { id: 3 },
+    };
+
+    var showPostHandler = {
+      serialize: function(object) {
+        return (object && { id: object.id }) || null;
+      },
+
+      model: function(params) {
+        return posts[params.id];
+      },
+    };
+
+    handlers = {
+      showPost: showPostHandler,
+    };
+
+    // Get a reference to the transition, mid-transition.
+    router.willTransition = function() {
+      var midTransitionState = router.activeTransition.state;
+
+      // Make sure that the activeIntent doesn't match post 300.
+      var isPost300Targeted = router.isActiveIntent(
+        'showPost',
+        [300],
+        null,
+        midTransitionState
+      );
+      assert.notOk(isPost300Targeted, 'Post 300 should not match post 3.');
+    };
+
+    // Go to post 3. This triggers our test.
+    transitionTo(router, '/posts/3');
+
+    // Clean up.
+    delete router.willTransition;
+  });
+
   test('tests whether arguments to transitionTo are considered active', function(assert) {
     var admin = { id: 47 },
       adminPost = { id: 74 },