Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent context matching if the oldContext is null #242

Merged
merged 4 commits into from
Mar 7, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 11 additions & 20 deletions lib/router/handler-info.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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))
);
}
Expand Down Expand Up @@ -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.
Expand Down
6 changes: 5 additions & 1 deletion lib/router/transition-intent/named-transition-intent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
42 changes: 42 additions & 0 deletions tests/router_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand Down