Skip to content

Commit

Permalink
Fix queryParamsOnly being correct on transitions.
Browse files Browse the repository at this point in the history
This property has been set incorrectly on some transitions, notably when the
transition is caused by the Router#refresh() method in queryParamsDidChange.

This is the method ember uses to implement `refreshModel` option of query params,
so fixing this bug allows `refreshModel` to be used in conjunction with
`replace` option for query params, currently it is either-or.
  • Loading branch information
alexspeller committed Oct 29, 2016
1 parent 7759a11 commit 437fd1a
Show file tree
Hide file tree
Showing 2 changed files with 106 additions and 0 deletions.
51 changes: 51 additions & 0 deletions lib/router/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ function getTransitionByIntent(intent, isIntermediate) {
if (queryParamChangelist) {
newTransition = this.queryParamsTransition(queryParamChangelist, wasTransitioning, oldState, newState);
if (newTransition) {
newTransition.queryParamsOnly = true;
return newTransition;
}
}
Expand All @@ -61,6 +62,12 @@ function getTransitionByIntent(intent, isIntermediate) {
// Create a new transition to the destination route.
newTransition = new Transition(this, intent, newState);

// transition is to same route with same params, only query params differ.
// not caught above probably because refresh() has been used
if ( handlerInfosSameExceptQueryParams(newState.handlerInfos, oldState.handlerInfos ) ) {
newTransition.queryParamsOnly = true;
}

// Abort and usurp any previously active transition.
if (this.activeTransition) {
this.activeTransition.abort();
Expand Down Expand Up @@ -750,6 +757,50 @@ function handlerInfosEqual(handlerInfos, otherHandlerInfos) {
return true;
}

function handlerInfosSameExceptQueryParams(handlerInfos, otherHandlerInfos) {
if (handlerInfos.length !== otherHandlerInfos.length) {
return false;
}

for (var i = 0, len = handlerInfos.length; i < len; ++i) {
if (handlerInfos[i].name !== otherHandlerInfos[i].name) {
return false;
}

if (!paramsEqual(handlerInfos[i].params, otherHandlerInfos[i].params)) {
return false;
}
}
return true;

}

function paramsEqual(params, otherParams) {
if (!params && !otherParams) {
return true;
} else if (!params && !!otherParams || !!params && !otherParams) {
// one is falsy but other is not;
return false;
}
var keys = Object.keys(params);
var otherKeys = Object.keys(otherParams);

if (keys.length !== otherKeys.length) {
return false;
}

for (var i = 0, len = keys.length; i < len; ++i) {
var key = keys[i];

if ( params[key] !== otherParams[key] ) {
return false;
}
}

return true;

}

function finalizeQueryParamChange(router, resolvedHandlers, newQueryParams, transition) {
// We fire a finalizeQueryParamChange event which
// gives the new route hierarchy a chance to tell
Expand Down
55 changes: 55 additions & 0 deletions test/tests/query_params_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,61 @@ test("transitioning between routes fires a queryParamsDidChange event", function

});


test("Refreshing the route when changing only query params should correctly set queryParamsOnly", function(assert) {
assert.expect(10);

var initialTransition = true;

handlers.index = {
events: {
finalizeQueryParamChange: function(params, finalParams, transition) {
if (initialTransition) {
assert.notOk(transition.queryParamsOnly, 'should not be query params only transition');
initialTransition = false;
} else {
assert.ok(transition.queryParamsOnly, 'should be query params only transition');
}
},

queryParamsDidChange: function() {
router.refresh();
}
}
};

handlers.child = {
events: {
finalizeQueryParamChange: function(params, finalParams, transition) {
assert.notOk(transition.queryParamsOnly, 'should be normal transition');
return true;
},
}
};

var transition = transitionTo(router, '/index');
assert.notOk(transition.queryParamsOnly, "Initial transition is not query params only transition");

transition = transitionTo(router, '/index?foo=123');

assert.ok(transition.queryParamsOnly, "Second transition with updateURL intent is query params only");

transition = router.replaceWith('/index?foo=456');
flushBackburner();

assert.ok(transition.queryParamsOnly, "Third transition with replaceURL intent is query params only");


transition = transitionTo(router, '/parent/child?foo=789');
assert.notOk(transition.queryParamsOnly, "Fourth transition with transtionTo intent is not query params only");

transition = transitionTo(router, '/parent/child?foo=901');
assert.ok(transition.queryParamsOnly, "Firth transition with transtionTo intent is query params only");

transition = transitionTo(router, '/index?foo=123');
assert.notOk(transition.queryParamsOnly, "Firth transition with transtionTo intent is not query params only");
});

test("a handler can opt into a full-on transition by calling refresh", function(assert) {
assert.expect(3);

Expand Down

0 comments on commit 437fd1a

Please sign in to comment.