diff --git a/lib/router/router.ts b/lib/router/router.ts index 5de1b895..6e7cb4c2 100644 --- a/lib/router/router.ts +++ b/lib/router/router.ts @@ -120,9 +120,19 @@ export default abstract class Router { // 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 { 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); diff --git a/lib/router/transition.ts b/lib/router/transition.ts index 43cd3264..a2d8e3aa 100644 --- a/lib/router/transition.ts +++ b/lib/router/transition.ts @@ -63,6 +63,7 @@ export default class Transition implements Partial> isCausedByAbortingTransition = false; isCausedByInitialTransition = false; isCausedByAbortingReplaceTransition = false; + isCausedByUpdateTransition = false; _visibleQueryParams: Dict = {}; constructor( @@ -107,6 +108,12 @@ export default class Transition implements Partial> (!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; diff --git a/tests/query_params_test.ts b/tests/query_params_test.ts index 61059f56..99a2d49a 100644 --- a/tests/query_params_test.ts +++ b/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, finalParams: Dict[]) { - 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); diff --git a/tests/router_test.ts b/tests/router_test.ts index 5a5e7858..cab74c15 100644 --- a/tests/router_test.ts +++ b/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) { + // Pass query params through to `afterModel` hook + return params.queryParams; + }, + afterModel(model: Dict) { + // 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) { + // Pass query params through to `afterModel` hook + return params.queryParams; + }, + afterModel(model: Dict) { + // 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'); diff --git a/tests/test_helpers.ts b/tests/test_helpers.ts index 29922b87..c4d4d420 100644 --- a/tests/test_helpers.ts +++ b/tests/test_helpers.ts @@ -229,3 +229,12 @@ export function trigger( throw new Error("Nothing handled the event '" + name + "'."); } } + +export function consumeAllFinalQueryParams(params: Dict, finalParams: Dict[]) { + for (let key in params) { + let value = params[key]; + delete params[key]; + finalParams.push({ key: key, value: value }); + } + return true; +}