Skip to content

Commit

Permalink
Fix validation stage redirection behaviour.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
alexspeller committed Oct 31, 2016
1 parent 7759a11 commit 3723643
Show file tree
Hide file tree
Showing 3 changed files with 705 additions and 14 deletions.
25 changes: 23 additions & 2 deletions lib/router/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down
40 changes: 28 additions & 12 deletions lib/router/transition.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 comment has been minimized.

Copy link
@alanwguo

alanwguo Jan 13, 2017

@alexspeller, I have an issue here:
I have a case where I do a replaceWith to the same route with different queryParams.
The route has the queryParams configuration { replace: true }.

The queryParamsDidChange method calls this.refresh() during the replaceWith transition which aborts the previous transition. Because this.isCausedByAbortingTransition is true, however, we no longer respect the original replaceWith and now do an updateUrl.

This comment has been minimized.

Copy link
@alanwguo

alanwguo Jan 13, 2017

Perhaps in the originating replaceWith call, we force the replaceWith to happen by having a forceReplace boolean on the transition. Or a bit more hacky: by setting isCausedByAboritingTransition to false.

This comment has been minimized.

Copy link
@alexspeller

alexspeller Jan 13, 2017

Author Contributor

@alanwguo I have a PR open that sounds like it could be related - can you confirm if that PR helps with this issue? #198

This comment has been minimized.

Copy link
@alanwguo

alanwguo Jan 13, 2017

I did a quick search of queryParamsOnly and I don't believe it will affect the isCausedByAbortingTransition value. It seems that this may be a separate issue since even if the transition method is correctly set as replace, the finalizeTransition method will still elect to do an updateUrl because replaceAndNotAborting is false.

relevant code:

var replaceAndNotAborting = (
  urlMethod === 'replace' &&
  !transition.isCausedByAbortingTransition
);

This comment has been minimized.

Copy link
@nathanhammond

nathanhammond Jan 13, 2017

Contributor

@alanwguo Can you open an issue with a link to a repository which demonstrates this failure? Or, even better, a PR with a failing test?

This comment has been minimized.

Copy link
@alanwguo

alanwguo Jan 14, 2017

@nathanhammond Sure. here it is: #205

this.isCausedByInitialTransition = (
previousTransition && (
previousTransition.isCausedByInitialTransition ||
previousTransition.sequence === 0
)
);

if (state) {
this.params = state.params;
this.queryParams = state.queryParams;
Expand All @@ -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 = {};
Expand All @@ -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,
Expand Down
Loading

0 comments on commit 3723643

Please sign in to comment.