Fix URL Update Method for QP-replacement during Transition

lib/router/router.ts

@@ -120,9 +120,19 @@ export default abstract class Router<T extends Route> {
       // perform a URL update at the end. This gives
       // the user the ability to set the url update
       // method (default is replaceState).
-      let newTransition = new InternalTransition(this, undefined, undefined);
+      let newTransition = new InternalTransition(
+        this,
+        undefined,
+        undefined,
+        undefined,
+        this.activeTransition
+      );
       newTransition.queryParamsOnly = true;
 
+      if (this.activeTransition) {
+        this.activeTransition.redirect(newTransition);
+      }
+
       oldState.queryParams = this.finalizeQueryParamChange(
         newState.routeInfos,
         newState.queryParams,
@@ -699,7 +709,16 @@ export default abstract class Router<T extends Route> {
       let replacingReplace =
         urlMethod === 'replace' && transition.isCausedByAbortingReplaceTransition;
 
-      if (initial || replaceAndNotAborting || isQueryParamsRefreshTransition || replacingReplace) {
+      // If this is the initial transition and we cancelled it to add query params, we should
+      // *not* use the the original `replace` method; we need to fully update the URL to the initial
+      // URL _plus_ the new query params
+      let shouldHonorOriginalTransitionUpdateMethod =
+        urlMethod === 'replace' && transition.isCausedByUpdateTransition;
+
+      if (
+        !shouldHonorOriginalTransitionUpdateMethod &&
+        (initial || replaceAndNotAborting || isQueryParamsRefreshTransition || replacingReplace)
+      ) {
         this.replaceURL!(url);
       } else {
         this.updateURL(url);

lib/router/transition.ts

@@ -63,6 +63,7 @@ export default class Transition<T extends Route> implements Partial<Promise<T>>
   isCausedByAbortingTransition = false;
   isCausedByInitialTransition = false;
   isCausedByAbortingReplaceTransition = false;
+  isCausedByUpdateTransition = false;
   _visibleQueryParams: Dict<unknown> = {};
 
   constructor(
@@ -107,6 +108,12 @@ export default class Transition<T extends Route> implements Partial<Promise<T>>
       (!previousTransition.isCausedByAbortingTransition ||
         previousTransition.isCausedByAbortingReplaceTransition);
 
+    // This transition was caused by one that intended to update the URL if the previous transition
+    // would have updated the URL
+    this.isCausedByUpdateTransition =
+      !!previousTransition &&
+      (previousTransition.isCausedByUpdateTransition || previousTransition.urlMethod === 'update');
+
     if (state) {
       this[PARAMS_SYMBOL] = state.params;
       this[QUERY_PARAMS_SYMBOL] = state.queryParams;

tests/query_params_test.ts

@@ -4,6 +4,7 @@ import { Dict, Maybe } from 'router/core';
 import RouteInfo from 'router/route-info';
 import { Promise } from 'rsvp';
 import {
+  consumeAllFinalQueryParams,
   createHandler,
   flushBackburner,
   module,
@@ -78,15 +79,6 @@ scenarios.forEach(function (scenario) {
     router.map(fn);
   }
 
-  function consumeAllFinalQueryParams(params: Dict<unknown>, finalParams: Dict<unknown>[]) {
-    for (let key in params) {
-      let value = params[key];
-      delete params[key];
-      finalParams.push({ key: key, value: value });
-    }
-    return true;
-  }
-
   test('a change in query params fires a queryParamsDidChange event', function (assert) {
     assert.expect(7);
 

tests/router_test.ts

@@ -11,6 +11,7 @@ import { TransitionError } from 'router/transition-state';
 import { Promise, reject } from 'rsvp';
 import {
   assertAbort,
+  consumeAllFinalQueryParams,
   createHandler,
   flushBackburner,
   handleURL,
@@ -6225,6 +6226,97 @@ scenarios.forEach(function (scenario) {
     assert.equal(barModelCount, 1, 'Bar model should be called once');
   });
 
+  test('Calling transitionTo with the `replace` method during initial transition with only query params changes should use updateURL', function (assert) {
+    map(assert, function (match) {
+      match('/first').to('first');
+    });
+
+    routes.first = createHandler('first', {
+      model(params: Dict<unknown>) {
+        // Pass query params through to `afterModel` hook
+        return params.queryParams;
+      },
+      afterModel(model: Dict<unknown>) {
+        // Ensure we only redirect the first time through here
+        if (!model.foo) {
+          router
+            .transitionTo('first', {
+              queryParams: {
+                foo: 'bar',
+              },
+            })
+            .method('replace');
+        }
+      },
+
+      events: {
+        // Ensure query params are considered in transition
+        finalizeQueryParamChange: consumeAllFinalQueryParams,
+      },
+    });
+
+    router.updateURL = (url) => {
+      assert.step(`Updated the URL to ${url}`);
+    };
+
+    router.replaceURL = (url) => {
+      assert.step(`Replaced the URL with ${url}`);
+    };
+
+    transitionTo(router, 'first');
+    flushBackburner();
+
+    assert.verifySteps(['Updated the URL to /first?foo=bar']);
+  });
+
+  test('Calling transitionTo with the `replace` method after initial transition with only query params changes should use updateURL', function (assert) {
+    map(assert, function (match) {
+      match('/first').to('first');
+      match('/second').to('second');
+    });
+
+    routes.first = createHandler('first');
+    routes.second = createHandler('second', {
+      model(params: Dict<unknown>) {
+        // Pass query params through to `afterModel` hook
+        return params.queryParams;
+      },
+      afterModel(model: Dict<unknown>) {
+        // Ensure we only redirect the first time through here
+        if (!model.foo) {
+          router
+            .transitionTo('second', {
+              queryParams: {
+                foo: 'bar',
+              },
+            })
+            .method('replace');
+        }
+      },
+
+      events: {
+        // Ensure query params are considered in transition
+        finalizeQueryParamChange: consumeAllFinalQueryParams,
+      },
+    });
+
+    router.updateURL = (url) => {
+      assert.step(`Updated the URL to ${url}`);
+    };
+
+    router.replaceURL = (url) => {
+      assert.step(`Replaced the URL with ${url}`);
+    };
+
+    transitionTo(router, 'first');
+    flushBackburner();
+
+    transitionTo(router, 'second');
+    flushBackburner();
+
+    assert.verifySteps(['Updated the URL to /first', 'Updated the URL to /second?foo=bar']);
+  });
+
   test('transitioning to the same route with different context should not reenter the route', function (assert) {
     map(assert, function (match) {
       match('/project/:project_id').to('project');

tests/test_helpers.ts

@@ -229,3 +229,12 @@ export function trigger(
     throw new Error("Nothing handled the event '" + name + "'.");
   }
 }
+
+export function consumeAllFinalQueryParams(params: Dict<unknown>, finalParams: Dict<unknown>[]) {
+  for (let key in params) {
+    let value = params[key];
+    delete params[key];
+    finalParams.push({ key: key, value: value });
+  }
+  return true;
+}