Merge pull request #210 from alexspeller/fix-fracking-query-params-re…

…fresh-replace-again Fix query params refresh transitions if they are using replaceState
tildeio · Apr 25, 2017 · 7abcdca · 7abcdca
2 parents e0b58d1 + 2be1ec3
commit 7abcdca
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 5 deletions.
diff --git a/lib/router/router.js b/lib/router/router.js
@@ -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) {
@@ -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;
   },
 
   /**
@@ -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);

diff --git a/test/tests/query_params_test.js b/test/tests/query_params_test.js
@@ -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) {
@@ -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");