From 4903a46cafda5a4889ca1dd4d207255a286b6445 Mon Sep 17 00:00:00 2001 From: Nathan Hammond Date: Mon, 26 Feb 2018 17:10:35 -0800 Subject: [PATCH 1/4] Failing test for mid-transition active checks. --- tests/router_test.js | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/router_test.js b/tests/router_test.js index bc6e3da8..35f5f21e 100644 --- a/tests/router_test.js +++ b/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 }, From 7fe6e02f01d7d4b89e613bcdfcaa7e8bdefc1c6b Mon Sep 17 00:00:00 2001 From: Nathan Hammond Date: Mon, 26 Feb 2018 17:14:53 -0800 Subject: [PATCH 2/4] Prevent mid-transition checks from reassigning params. --- lib/router/transition-intent/named-transition-intent.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/router/transition-intent/named-transition-intent.js b/lib/router/transition-intent/named-transition-intent.js index 37e3147a..4247998b 100644 --- a/lib/router/transition-intent/named-transition-intent.js +++ b/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 && + oldHandlerInfo.hasOwnProperty('context') && + 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 From 28458cc4ab0ffbb35310343b47da33e19d1669de Mon Sep 17 00:00:00 2001 From: Nathan Hammond Date: Wed, 7 Mar 2018 10:45:08 -0800 Subject: [PATCH 3/4] Remove the context from the prototype. --- lib/router/handler-info.js | 29 ++++++++++------------------- 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/lib/router/handler-info.js b/lib/router/handler-info.js index f7a3266c..6975b117 100644 --- a/lib/router/handler-info.js +++ b/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 (this.hasOwnProperty('context') || !contextsMatch) { + resolution.context = resolvedContext; + } + + return this.factory('resolved', resolution); } shouldSupercede(other) { @@ -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. From d349e75aa2aedea2d0b8a156fdba47afb8f9ad47 Mon Sep 17 00:00:00 2001 From: Nathan Hammond Date: Wed, 7 Mar 2018 12:28:20 -0800 Subject: [PATCH 4/4] Replace hasOwnProperty with in since we have no prototype value. --- lib/router/handler-info.js | 4 ++-- lib/router/transition-intent/named-transition-intent.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/router/handler-info.js b/lib/router/handler-info.js index 6975b117..1839476e 100644 --- a/lib/router/handler-info.js +++ b/lib/router/handler-info.js @@ -187,7 +187,7 @@ export default class HandlerInfo { // Don't set a context on the resolution unless we actually have one. var contextsMatch = resolvedContext === this.context; - if (this.hasOwnProperty('context') || !contextsMatch) { + if ('context' in this || !contextsMatch) { resolution.context = resolvedContext; } @@ -208,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)) ); } diff --git a/lib/router/transition-intent/named-transition-intent.js b/lib/router/transition-intent/named-transition-intent.js index 4247998b..7abe6f55 100644 --- a/lib/router/transition-intent/named-transition-intent.js +++ b/lib/router/transition-intent/named-transition-intent.js @@ -116,7 +116,7 @@ export default class NamedTransitionIntent extends TransitionIntent { var oldContext = oldHandlerInfo && oldHandlerInfo.context; if ( result.names.length > 0 && - oldHandlerInfo.hasOwnProperty('context') && + 'context' in oldHandlerInfo && newHandlerInfo.context === oldContext ) { // If contexts match in isActive test, assume params also match.