From 22a2afd69ee57d8e27df799f24ff86dc3edfeec6 Mon Sep 17 00:00:00 2001 From: Alex Speller Date: Sat, 29 Oct 2016 14:46:28 +0200 Subject: [PATCH] Fix validation stage redirection behaviour. This has been broken forever AFAIK, and is surprising to a lot of people. In fact, even the ember guides recommend [using `this.transitionTo` for redirecting](https://guides.emberjs.com/v2.9.0/routing/redirection/) This is in fact broken. If you use `transitionTo` as a redirect strategy, then you get an extra history entry if you hit the route as the first route in your app. This breaks the back button, as the back button takes you to the route with the redirect, then immediately redirects back to the page you're on. Maybe you can go back if you hammer back button really quickly, but you have to hammer it loads super quick. Not a good UX. `replaceWith` works if you use it to redirect from a route that's your initial transition. However if you use it to redirect and you hit that route from some way _after_ the initial app load, then at the point that the replaceWith is called, you are still on the same URL. For example, if you are on `/` and you click a link to `/foo`, which in it's model hook redirects to `/bar` using `replaceWith`, at the time replaceWith is called, your current url is `/`. This means `/` entry gets removed from your history entirely. Clicking back will take you back to whatever page you were on before `/`, which often isn't event your app, maybe it's google or something. This breaks the back button again. This commit should do the correct thing in all cases, allowing replaceWith and transitionTo outside of redirects as specified by the developer but only allowing transitionTo or replaceWith in redirects in a way that doesn't break the back button. --- lib/router/router.js | 25 +- lib/router/transition.js | 40 ++- test/tests/router_test.js | 654 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 705 insertions(+), 14 deletions(-) diff --git a/lib/router/router.js b/lib/router/router.js index ecbc0349..974b1aa7 100644 --- a/lib/router/router.js +++ b/lib/router/router.js @@ -26,6 +26,7 @@ function Router(_options) { this.oldState = undefined; this.currentHandlerInfos = undefined; this.state = undefined; + this.currentSequence = 0; this.recognizer = new RouteRecognizer(); this.reset(); @@ -59,7 +60,7 @@ function getTransitionByIntent(intent, isIntermediate) { } // Create a new transition to the destination route. - newTransition = new Transition(this, intent, newState); + newTransition = new Transition(this, intent, newState, undefined, this.activeTransition); // Abort and usurp any previously active transition. if (this.activeTransition) { @@ -625,7 +626,27 @@ function updateURL(transition, state/*, inputUrl*/) { params.queryParams = transition._visibleQueryParams || state.queryParams; var url = router.recognizer.generate(handlerName, params); - if (urlMethod === 'replace') { + // transitions during the initial transition must always use replaceURL. + // When the app boots, you are at a url, e.g. /foo. If some handler + // redirects to bar as part of the initial transition, you don't want to + // add a history entry for /foo. If you do, pressing back will immediately + // hit the redirect again and take you back to /bar, thus killing the back + // button + var initial = transition.isCausedByInitialTransition; + + // say you are at / and you click a link to route /foo. In /foo's + // handler, the transition is aborted using replacewith('/bar'). + // Because the current url is still /, the history entry for / is + // removed from the history. Clicking back will take you to the page + // you were on before /, which is often not even the app, thus killing + // the back button. That's why updateURL is always correct for an + // aborting transition that's not the initial transition + var replaceAndNotAborting = ( + urlMethod === 'replace' && + !transition.isCausedByAbortingTransition + ); + + if (initial || replaceAndNotAborting) { router.replaceURL(url); } else { router.updateURL(url); diff --git a/lib/router/transition.js b/lib/router/transition.js index 328f0c27..887a997e 100644 --- a/lib/router/transition.js +++ b/lib/router/transition.js @@ -16,7 +16,7 @@ import { trigger, slice, log, promiseLabel } from './utils'; @param {Object} error @private */ -function Transition(router, intent, state, error) { +function Transition(router, intent, state, error, previousTransition) { var transition = this; this.state = state || router.state; this.intent = intent; @@ -40,6 +40,18 @@ function Transition(router, intent, state, error) { return; } + // if you're doing multiple redirects, need the new transition to know if it + // is actually part of the first transition or not. Any further redirects + // in the initial transition also need to know if they are part of the + // initial transition + this.isCausedByAbortingTransition = !!previousTransition; + this.isCausedByInitialTransition = ( + previousTransition && ( + previousTransition.isCausedByInitialTransition || + previousTransition.sequence === 0 + ) + ); + if (state) { this.params = state.params; this.queryParams = state.queryParams; @@ -58,16 +70,9 @@ function Transition(router, intent, state, error) { this.pivotHandler = handlerInfo.handler; } - this.sequence = Transition.currentSequence++; - this.promise = state.resolve(checkForAbort, this)['catch'](function(result) { - if (result.wasAborted || transition.isAborted) { - return Promise.reject(logAbort(transition)); - } else { - transition.trigger('error', result.error, transition, result.handlerWithError); - transition.abort(); - return Promise.reject(result.error); - } - }, promiseLabel('Handle Abort')); + this.sequence = router.currentSequence++; + this.promise = state.resolve(checkForAbort, this)['catch']( + catchHandlerForTransition(transition), promiseLabel('Handle Abort')); } else { this.promise = Promise.resolve(this.state); this.params = {}; @@ -80,7 +85,18 @@ function Transition(router, intent, state, error) { } } -Transition.currentSequence = 0; +function catchHandlerForTransition(transition) { + return function(result) { + if (result.wasAborted || transition.isAborted) { + return Promise.reject(logAbort(transition)); + } else { + transition.trigger('error', result.error, transition, result.handlerWithError); + transition.abort(); + return Promise.reject(result.error); + } + }; +} + Transition.prototype = { targetName: null, diff --git a/test/tests/router_test.js b/test/tests/router_test.js index f40458c6..d2d350f3 100644 --- a/test/tests/router_test.js +++ b/test/tests/router_test.js @@ -3327,6 +3327,660 @@ test("intermediateTransitionTo() forces an immediate intermediate transition tha counterAt(7, "original transition promise resolves"); }); + +test("Calling transitionTo during initial transition in validation hook should use replaceURL", function(assert) { + assert.expect(4); + map(assert, function(match) { + match("/foo").to('foo'); + match("/bar").to('bar'); + }); + + var fooModelCount = 0, barModelCount = 0; + + router.updateURL = function(updateUrl) { + url = updateUrl; + assert.ok(false, "The url was not correctly replaced on initial transition"); + }; + + router.replaceURL = function(replaceURL) { + url = replaceURL; + assert.ok(true, "The url was replaced correctly on initial transition"); + }; + + var fooHandler = { + model: function() { + fooModelCount++; + router.transitionTo('/bar'); + } + }; + + var barHandler = { + model: function() { + barModelCount++; + } + }; + + handlers = { + foo: fooHandler, + bar: barHandler + }; + + transitionTo(router, '/foo'); + + assert.equal(url, '/bar'); + assert.equal(fooModelCount, 1); + assert.equal(barModelCount, 1); +}); + +test("Calling transitionTo during initial transition in validation hook with multiple redirects should use replaceURL", function(assert) { + assert.expect(5); + map(assert, function(match) { + match("/foo").to('foo'); + match("/bar").to('bar'); + match("/baz").to('baz'); + }); + + var fooModelCount = 0, barModelCount = 0, bazModelCount = 0; + + router.updateURL = function(updateUrl) { + url = updateUrl; + assert.ok(false, "The url was not correctly replaced on initial transition"); + }; + + router.replaceURL = function(replaceURL) { + url = replaceURL; + assert.ok(true, "The url was replaced correctly on initial transition"); + }; + + var fooHandler = { + model: function() { + fooModelCount++; + router.transitionTo('/bar'); + } + }; + + var barHandler = { + model: function() { + barModelCount++; + router.transitionTo('/baz'); + } + }; + + var bazHandler = { + model: function() { + bazModelCount++; + } + }; + + handlers = { + foo: fooHandler, + bar: barHandler, + baz: bazHandler + }; + + transitionTo(router, '/foo'); + + assert.equal(url, '/baz'); + assert.equal(fooModelCount, 1); + assert.equal(barModelCount, 1); + assert.equal(bazModelCount, 1); +}); + +test("Calling transitionTo after initial transition in validation hook should use updateUrl", function(assert) { + assert.expect(8); + + map(assert, function(match) { + match("/foo").to('foo'); + match("/bar").to('bar'); + }); + + var fooModelCount = 0, barModelCount = 0; + + router.updateURL = function(updateUrl) { + url = updateUrl; + assert.ok(true, "updateURL should be used"); + }; + + router.replaceURL = function(replaceURL) { + url = replaceURL; + assert.ok(false, "replaceURL should not be used"); + }; + + var fooHandler = { + model: function() { + fooModelCount++; + router.transitionTo('/bar'); + } + }; + + var barHandler = { + model: function() { + barModelCount++; + } + }; + + handlers = { + foo: fooHandler, + bar: barHandler + }; + + transitionTo(router, '/bar'); + + assert.equal(url, '/bar'); + assert.equal(barModelCount, 1, 'Bar model should be called once'); + assert.equal(fooModelCount, 0, 'Foo model should not be called'); + + transitionTo(router, '/foo'); + + assert.equal(url, '/bar'); + assert.equal(barModelCount, 2, 'Bar model should be called twice'); + assert.equal(fooModelCount, 1, 'Foo model should be called once'); +}); + +test("Calling transitionTo after initial transition in validation hook with multiple redirects should use updateUrl", function(assert) { + assert.expect(10); + + map(assert, function(match) { + match('/foo').to('foo'); + match('/bar').to('bar'); + match('/baz').to('baz'); + }); + + var fooModelCount = 0, barModelCount = 0, bazModelCount = 0; + + router.updateURL = function(updateUrl) { + url = updateUrl; + assert.ok(true, "updateURL should be used"); + }; + + router.replaceURL = function(replaceURL) { + url = replaceURL; + assert.ok(false, "replaceURL should not be used"); + }; + + var fooHandler = { + model: function() { + fooModelCount++; + router.transitionTo('/bar'); + } + }; + + var barHandler = { + model: function() { + barModelCount++; + router.transitionTo('/baz'); + } + }; + + var bazHandler = { + model: function() { + bazModelCount++; + } + }; + + handlers = { + foo: fooHandler, + bar: barHandler, + baz: bazHandler + }; + + transitionTo(router, '/baz'); + + assert.equal(url, '/baz'); + assert.equal(bazModelCount, 1, 'Baz model should be called once'); + assert.equal(fooModelCount, 0, 'Foo model should not be called'); + assert.equal(barModelCount, 0, 'Bar model should not be called'); + + transitionTo(router, '/foo'); + + assert.equal(url, '/baz'); + assert.equal(bazModelCount, 2, 'Baz model should be called twice'); + assert.equal(fooModelCount, 1, 'Foo model should be called once'); + assert.equal(barModelCount, 1, 'Bar model should be called once'); +}); + + +test("Calling replaceWith during initial transition in validation hook should use replaceURL", function(assert) { + assert.expect(4); + map(assert, function(match) { + match("/foo").to('foo'); + match("/bar").to('bar'); + }); + + var fooModelCount = 0, barModelCount = 0; + + router.updateURL = function(updateUrl) { + url = updateUrl; + assert.ok(false, "The url was not correctly replaced on initial transition"); + }; + + router.replaceURL = function(replaceURL) { + url = replaceURL; + assert.ok(true, "The url was replaced correctly on initial transition"); + }; + + var fooHandler = { + model: function() { + fooModelCount++; + router.replaceWith('/bar'); + } + }; + + var barHandler = { + model: function() { + barModelCount++; + } + }; + + handlers = { + foo: fooHandler, + bar: barHandler + }; + + transitionTo(router, '/foo'); + + assert.equal(url, '/bar'); + assert.equal(fooModelCount, 1); + assert.equal(barModelCount, 1); +}); + +test("Calling replaceWith during initial transition in validation hook with multiple redirects should use replaceURL", function(assert) { + assert.expect(5); + map(assert, function(match) { + match("/foo").to('foo'); + match("/bar").to('bar'); + match("/baz").to('baz'); + }); + + var fooModelCount = 0, barModelCount = 0, bazModelCount = 0; + + router.updateURL = function(updateUrl) { + url = updateUrl; + assert.ok(false, "The url was not correctly replaced on initial transition"); + }; + + router.replaceURL = function(replaceURL) { + url = replaceURL; + assert.ok(true, "The url was replaced correctly on initial transition"); + }; + + var fooHandler = { + model: function() { + fooModelCount++; + router.replaceWith('/bar'); + } + }; + + var barHandler = { + model: function() { + barModelCount++; + router.replaceWith('/baz'); + } + }; + + var bazHandler = { + model: function() { + bazModelCount++; + } + }; + + handlers = { + foo: fooHandler, + bar: barHandler, + baz: bazHandler + }; + + transitionTo(router, '/foo'); + + assert.equal(url, '/baz'); + assert.equal(fooModelCount, 1, 'should call foo model once'); + assert.equal(barModelCount, 1, 'should call bar model once'); + assert.equal(bazModelCount, 1, 'should call baz model once'); +}); + + +test("Calling replaceWith after initial transition in validation hook should use updateUrl", function(assert) { + assert.expect(8); + + map(assert, function(match) { + match("/foo").to('foo'); + match("/bar").to('bar'); + }); + + var fooModelCount = 0, barModelCount = 0; + + router.updateURL = function(updateUrl) { + url = updateUrl; + assert.ok(true, "updateURL should be used"); + }; + + router.replaceURL = function(replaceURL) { + url = replaceURL; + assert.ok(false, "replaceURL should not be used"); + }; + + var fooHandler = { + model: function() { + fooModelCount++; + router.replaceWith('/bar'); + } + }; + var barHandler = { + model: function() { + barModelCount++; + } + }; + + handlers = { + foo: fooHandler, + bar: barHandler + }; + + transitionTo(router, '/bar'); + + assert.equal(url, '/bar'); + assert.equal(barModelCount, 1, 'Bar model should be called once'); + assert.equal(fooModelCount, 0, 'Foo model should not be called'); + + transitionTo(router, '/foo'); + + assert.equal(url, '/bar'); + assert.equal(barModelCount, 2, 'Bar model should be called twice'); + assert.equal(fooModelCount, 1, 'Foo model should be called once'); +}); + +test("Calling replaceWith after initial transition in validation hook with multiple redirects should use updateUrl", function(assert) { + assert.expect(10); + + map(assert, function(match) { + match("/foo").to('foo'); + match("/bar").to('bar'); + match("/baz").to('baz'); + }); + + var fooModelCount = 0, barModelCount = 0, bazModelCount = 0; + + router.updateURL = function(updateUrl) { + url = updateUrl; + assert.ok(true, "updateURL should be used"); + }; + + router.replaceURL = function(replaceURL) { + url = replaceURL; + assert.ok(false, "replaceURL should not be used"); + }; + + var fooHandler = { + model: function() { + fooModelCount++; + router.replaceWith('/bar'); + } + }; + + var barHandler = { + model: function() { + barModelCount++; + router.replaceWith('/baz'); + } + }; + + var bazHandler = { + model: function() { + bazModelCount++; + } + }; + + handlers = { + foo: fooHandler, + bar: barHandler, + baz: bazHandler + }; + + transitionTo(router, '/baz'); + + assert.equal(url, '/baz'); + assert.equal(bazModelCount, 1, 'Bar model should be called once'); + assert.equal(fooModelCount, 0, 'Foo model should not be called'); + assert.equal(barModelCount, 0, 'Baz model should not be called'); + + transitionTo(router, '/foo'); + + assert.equal(url, '/baz'); + assert.equal(bazModelCount, 2, 'Baz model should be called twice'); + assert.equal(fooModelCount, 1, 'Foo model should be called once'); + assert.equal(barModelCount, 1, 'Bar model should be called once'); +}); + + +test("Mixing multiple types of redirect during initial transition should work", function(assert) { + assert.expect(10); + + map(assert, function(match) { + match("/foo").to('foo'); + match("/bar").to('bar'); + match("/baz").to('baz'); + }); + + var fooModelCount = 0, barModelCount = 0, bazModelCount = 0; + + router.updateURL = function(updateUrl) { + url = updateUrl; + assert.ok(true, "updateURL should be used"); + }; + + router.replaceURL = function(replaceURL) { + url = replaceURL; + assert.ok(false, "replaceURL should not be used"); + }; + + var fooHandler = { + model: function() { + fooModelCount++; + router.replaceWith('/bar'); + } + }; + + var barHandler = { + model: function() { + barModelCount++; + router.transitionTo('/baz'); + } + }; + + var bazHandler = { + model: function() { + bazModelCount++; + } + }; + + handlers = { + foo: fooHandler, + bar: barHandler, + baz: bazHandler + }; + + transitionTo(router, '/baz'); + + assert.equal(url, '/baz'); + assert.equal(bazModelCount, 1, 'Bar model should be called once'); + assert.equal(fooModelCount, 0, 'Foo model should not be called'); + assert.equal(barModelCount, 0, 'Baz model should not be called'); + + transitionTo(router, '/foo'); + + assert.equal(url, '/baz'); + assert.equal(bazModelCount, 2, 'Baz model should be called twice'); + assert.equal(fooModelCount, 1, 'Foo model should be called once'); + assert.equal(barModelCount, 1, 'Bar model should be called once'); +}); + +test("Mixing multiple types of redirects after initial transition should work", function(assert) { + assert.expect(12); + + map(assert, function(match) { + match("/foo").to('foo'); + match("/bar").to('bar'); + match("/baz").to('baz'); + }); + + var fooModelCount = 0, barModelCount = 0, bazModelCount = 0, updateUrlCount = 0, replaceUrlCount = 0; + + router.updateURL = function(updateUrl) { + url = updateUrl; + updateUrlCount++; + }; + + router.replaceURL = function(replaceURL) { + url = replaceURL; + replaceUrlCount++; + }; + + var fooHandler = { + model: function() { + fooModelCount++; + router.replaceWith('/bar'); + } + }; + + var barHandler = { + model: function() { + barModelCount++; + router.transitionTo('/baz'); + } + }; + + var bazHandler = { + model: function() { + bazModelCount++; + } + }; + + handlers = { + foo: fooHandler, + bar: barHandler, + baz: bazHandler + }; + + transitionTo(router, '/baz'); + // actually replaceURL probably makes more sense here, but it's an initial + // transition to a route that the page loaded on, so it's a no-op and doesn't + // cause a problem + assert.equal(replaceUrlCount, 0, 'replaceURL should not be used'); + assert.equal(updateUrlCount, 1, 'updateURL should be used for initial transition'); + assert.equal(url, '/baz'); + assert.equal(bazModelCount, 1, 'Baz model should be called once'); + assert.equal(fooModelCount, 0, 'Foo model should not be called'); + assert.equal(barModelCount, 0, 'Bar model should not be called'); + + transitionTo(router, '/foo'); + + assert.equal(replaceUrlCount, 0, 'replaceURL should not be used'); + assert.equal(updateUrlCount, 2, 'updateURL should be used for subsequent transition'); + assert.equal(url, '/baz'); + assert.equal(bazModelCount, 2, 'Baz model should be called twice'); + assert.equal(fooModelCount, 1, 'Foo model should be called once'); + assert.equal(barModelCount, 1, 'Bar model should be called once'); +}); + + +test("Calling replaceWith after initial transition outside validation hook should use replaceURL", function(assert) { + assert.expect(7); + + map(assert, function(match) { + match("/foo").to('foo'); + match("/bar").to('bar'); + }); + + var fooModelCount = 0, barModelCount = 0; + + router.updateURL = function(updateUrl) { + url = updateUrl; + assert.equal(updateUrl, '/foo', "incorrect url for updateURL"); + }; + + router.replaceURL = function(replaceUrl) { + url = replaceUrl; + assert.equal(replaceUrl, '/bar', "incorrect url for replaceURL"); + }; + + var fooHandler = { + model: function() { + fooModelCount++; + } + }; + var barHandler = { + model: function() { + barModelCount++; + } + }; + + handlers = { + foo: fooHandler, + bar: barHandler + }; + + transitionTo(router, '/foo'); + + assert.equal(url, '/foo', "failed initial transition"); + assert.equal(fooModelCount, 1, 'Foo model should be called once'); + assert.equal(barModelCount, 0, 'Bar model should not be called'); + + router.replaceWith('/bar'); + flushBackburner(); + + assert.equal(fooModelCount, 1, 'Foo model should be called once'); + assert.equal(barModelCount, 1, 'Bar model should be called once'); +}); + +test("Calling transitionTo after initial transition outside validation hook should use updateUrl", function(assert) { + assert.expect(7); + + map(assert, function(match) { + match("/foo").to('foo'); + match("/bar").to('bar'); + }); + + var fooModelCount = 0, barModelCount = 0; + + router.updateURL = function(updateUrl) { + url = updateUrl; + assert.ok(true, "updateURL is used"); + }; + + router.replaceURL = function(replaceURL) { + url = replaceURL; + assert.ok(false, "replaceURL should not be used"); + }; + + var fooHandler = { + model: function() { + fooModelCount++; + } + }; + var barHandler = { + model: function() { + barModelCount++; + } + }; + + handlers = { + foo: fooHandler, + bar: barHandler + }; + + transitionTo(router, '/foo'); + + assert.equal(url, '/foo', "failed initial transition"); + assert.equal(fooModelCount, 1, 'Foo model should be called once'); + assert.equal(barModelCount, 0, 'Bar model should not be called'); + + transitionTo(router, '/bar'); + + assert.equal(fooModelCount, 1, 'Foo model should be called once'); + assert.equal(barModelCount, 1, 'Bar model should be called once'); +}); + + 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');