From 325e34a9531d85da2456aa96ffa4412723681ac9 Mon Sep 17 00:00:00 2001 From: Alex LaFroscia Date: Fri, 14 Aug 2020 15:44:59 -0400 Subject: [PATCH 1/3] test: extract `consumeAllFinalQueryParams` helper --- tests/query_params_test.ts | 10 +--------- tests/test_helpers.ts | 9 +++++++++ 2 files changed, 10 insertions(+), 9 deletions(-) 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/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; +} From fc2177d52c15ae77c6f728108b98be0f08285b0e Mon Sep 17 00:00:00 2001 From: Alex LaFroscia Date: Fri, 14 Aug 2020 13:04:43 -0400 Subject: [PATCH 2/3] test: add failure for QP replacement transition --- tests/router_test.ts | 92 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) 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'); From 0702b719ea765ca72408b5d041ca0618b66beb82 Mon Sep 17 00:00:00 2001 From: Alex LaFroscia Date: Fri, 14 Aug 2020 16:23:38 -0400 Subject: [PATCH 3/3] fix: link interrupting QP-only transition to the active transition This makes a QP-only transition that updates an active transition actually replace the active transition, rather than them both happening. We also keep track of whether the interrupted transition was intending to update the URL, because a replacement that interrupts an update should still update the URL. --- lib/router/router.ts | 23 +++++++++++++++++++++-- lib/router/transition.ts | 7 +++++++ 2 files changed, 28 insertions(+), 2 deletions(-) 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;