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

Fix query params refresh transitions if they are using replaceState #210

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
22 changes: 19 additions & 3 deletions lib/router/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,8 @@ Router.prototype = {
},

refresh: function(pivotHandler) {
var state = this.activeTransition ? this.activeTransition.state : this.state;
var previousTransition = this.activeTransition;
var state = previousTransition ? previousTransition.state : this.state;
var handlerInfos = state.handlerInfos;
var params = {};
for (var i = 0, len = handlerInfos.length; i < len; ++i) {
Expand All @@ -261,7 +262,14 @@ Router.prototype = {
queryParams: this._changedQueryParams || state.queryParams || {}
});

return this.transitionByIntent(intent, false);
var newTransition = this.transitionByIntent(intent, false);

// if the previous transition is a replace transition, that needs to be preserved
if (previousTransition && previousTransition.urlMethod === 'replace') {
newTransition.method(previousTransition.urlMethod);
}

return newTransition;
},

/**
Expand Down Expand Up @@ -654,7 +662,15 @@ function updateURL(transition, state/*, inputUrl*/) {
!transition.isCausedByAbortingTransition
);

if (initial || replaceAndNotAborting) {
// because calling refresh causes an aborted transition, this needs to be
// special cased - if the initial transition is a replace transition, the
// urlMethod should be honored here.
var isQueryParamsRefreshTransition = (
transition.queryParamsOnly &&
urlMethod === 'replace'
);

if (initial || replaceAndNotAborting || isQueryParamsRefreshTransition) {
router.replaceURL(url);
} else {
router.updateURL(url);
Expand Down
19 changes: 17 additions & 2 deletions test/tests/query_params_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,22 @@ 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);
assert.expect(16);

var initialTransition = true;

var expectReplace;

router.updateURL = function(newUrl) {
assert.notOk(expectReplace, "Expected replace but update was called");
url = newUrl;
};

router.replaceURL = function(newUrl) {
assert.ok(expectReplace, "Replace was called but update was expected");
url = newUrl;
};

handlers.index = {
events: {
finalizeQueryParamChange: function(params, finalParams, transition) {
Expand All @@ -189,18 +201,21 @@ test("Refreshing the route when changing only query params should correctly set
}
};

expectReplace = false;

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");

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

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

expectReplace = false;

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