Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix validation stage redirection behaviour. #197

Merged
merged 1 commit into from
Nov 1, 2016
Merged

Fix validation stage redirection behaviour. #197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions lib/router/router.js
View file
Open in desktop
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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to the transition constructor now

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
View file
Open in desktop
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must we pass the entire transition forward? It appears that we only need to identify if this is the initial transition. As it stands it seems like this might be closing over a lot of transitions which would keep them dormant in memory longer than we care for.

(Need somebody more familiar to confirm/deny, just a quick thought.)

Copy link
Contributor Author

@alexspeller alexspeller Oct 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The transition isn't needed beyond setting two boolean properties in the constructor. Now my understanding is that because I'm not saving any reference to the transition after the constructor, it will be GC'ed the same place as it was before. Maybe someone with more js VM / GC knowledge can confirm

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree with @alexspeller here, but might make sense for @stefanpenner / @krisselden / other @tildeio/ember-core folks to sanity check also...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this doesn't affect retention.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexspeller confirm, you aren't retaining it so it does not affect GC

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexspeller I spoke too soon, there is a closure to this context being created.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing / double checking @krisselden.

@nathanhammond - Are you happy to move forward with that confirmation, or should we do more tests (or refactor anyways)?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on more detailed review, it looks like transition is the only property that would be added to the closure. It is a little weird to do this in a condition.

I'd likely make a function that took transitionas an arg that returned a error handler. This limits the scope and shape of the closure context.

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.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