-
Notifications
You must be signed in to change notification settings - Fork 159
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.
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
Fix validation stage redirection behaviour. #197
Conversation
@stefanpenner you can retire |
bb035cd
to
fbe33a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor things, but overall this seems solid. I appreciate the detail put into it.
// in the initial transition also need to know if they are part of the | ||
// initial transition | ||
newTransition.isCausedByInitialTransition = this.activeTransition.isCausedByInitialTransition|| this.activeTransition.sequence === 0; | ||
newTransition.isCausedByAbortingTransition = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we set these in the Transition constructor? At the very least, we should set them to undefined
in the constructor to avoid mutating the shape of the transition object.
// 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 | ||
newTransition.isCausedByInitialTransition = this.activeTransition.isCausedByInitialTransition|| this.activeTransition.sequence === 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Space before the ||
router.replaceURL(url); | ||
} else { | ||
router.updateURL(url); | ||
if (urlMethod === 'replace' && !aborting) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure how others will feel about this, but I'd prefer doing:
if (initial) {
} else if (urlMethod === 'replace' && !aborting) {
} else {
}
Instead of the nesting you currently have.
@@ -3327,6 +3329,672 @@ 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) { | |||
Transition.currentSequence = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tangential to this PR, we should probably store this on the Router instance if possible.
fbe33a7
to
e93f8ef
Compare
@@ -63,6 +63,13 @@ function getTransitionByIntent(intent, isIntermediate) { | |||
|
|||
// Abort and usurp any previously active transition. | |||
if (this.activeTransition) { |
There was a problem hiding this comment.
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
@trentmwillis thanks for detailed review, I made some improvements based on your comments, including moving currentSequence to router - that was bugging me too and it was a minor change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. This looks good to me, much easier to follow now I think.
!transition.isCausedByAbortingTransition | ||
); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Remove the extra line here
@alexspeller - What happens to folks that are already working around these bugs? I think that existing workarounds continue to work fine (but are just not needed), but I'd like to confirm. |
@rwjblue correct, this makes no difference to anyone using workarounds. This patch ensures that the right thing is always done. If the right thing is being done already there is no difference |
e93f8ef
to
bf4d075
Compare
@nathanhammond - r? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small things and one thing which needs discussion. I'm looking forward to retiring @stefanpenner's fix.
@@ -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) { |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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 transition
as an arg that returned a error handler. This limits the scope and shape of the closure context.
transitionTo(router, '/foo'); | ||
|
||
assert.equal(url, '/baz'); | ||
assert.equal(bazModelCount, 2, 'Baz model should be called thrice'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something is wrong on this line, unsure whether it's the message or the expectation.
|
||
router.replaceURL = function(replaceURL) { | ||
url = replaceURL; | ||
assert.ok(false, "replaceURL should not be used"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love how you're partially following ye ol' @tomhuda style of "English? double quotes. Code? single quotes."
If you want to map all quote behavior in a specific way that might be worth doing. I didn't review the non-diff so I don't know how consistent it is currently.
assert.equal(updateUrlCount, 2, 'updateURL should be used for subsequent transition'); | ||
assert.equal(url, '/baz'); | ||
assert.equal(bazModelCount, 2, 'Baz model should be called once'); | ||
assert.equal(fooModelCount, 1, 'Foo model should not be called'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one and the line below it need messaging adjustments.
bf4d075
to
e97244f
Compare
Fixed test labelling, think I've addressed saving previous transition issue, not addressing quoting style currently |
@@ -58,7 +70,7 @@ function Transition(router, intent, state, error) { | |||
this.pivotHandler = handlerInfo.handler; | |||
} | |||
|
|||
this.sequence = Transition.currentSequence++; | |||
this.sequence = router.currentSequence++; | |||
this.promise = state.resolve(checkForAbort, this)['catch'](function(result) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this function catches the transition in the closure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, could be (stupid VM's doing smart things). Without looking terribly deeply here, it seems that we might be able to avoid using closure scope stuff (and avoid generating a new function for each transition created too), by elevating this to a private prototype method (and moving checkForAbort
) and calling it from the constructor..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still concerned about this accidentally trolling us in the future. One of us won't be paying attention and will merge a PR which accidentally again captures things in a closure. In general I'm hesitant to pass around "huge" (by reference) objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. We definitely need to figure out the flags to know which method to use, do you have suggestions that would allow us to accomplish both objectives?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually right now I'm just being a jerk and identifying problems without coming up with proposed solutions. 😛 Addressing this one problem (for example, as you proposed) and then adding a comment to remind us to look out for this behavior is probably just fine.
I'd still like some GC/VM wizards to chime in on anything else we may have missed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So to be clear, right now the best thing for me to do is to refactor that function in the transition constructor to a prototype method, so that it doesn't risk closing over the arguments passed in to the transition constructor?
FWIW I hadn't thought that it would close over variables that you don't explicitly use in the function body, however that's based on chrome's behaviour and not a deep understanding of the various js VMs ember supports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That function is lexically scoped in a place where it can reach the previous transition which means it can't be cleaned up until that property is reset.
Yes, we need to refactor it such that the definition of this section happens outside of the constructor. This will have the bonus win of being more performant.
e97244f
to
22a2afd
Compare
@krisselden I believe this is what you meant, avoiding the creation of a closure in the constructor now @rwjblue @nathanhammond @trentmwillis AFAIK this should be ready for merge based on the feedback given above, let me know if there's anything else 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working through that with us @alexspeller (lots of learning for me on this one too).
@nathanhammond - r?
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.
22a2afd
to
3723643
Compare
@alexspeller Thank you for keeping this thing moving with all of the deep JS internals review. I'm grateful for the detailed attention you paid to this and for getting this solved at the appropriate layer. @krisselden I always learn things when you show up on threads, I appreciate your detailed analysis of what's going on in JS Engine land both in this thread and on Slack. This change now appears to be rock solid, let land it! 🎉 |
So I fixed this in tildeio#198. But it was broken again by tildeio#197 and the tests didn't catch that. So I've improved the tests and fixed it again.
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 redirectingThis in fact does not work. 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 yourinitial 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 backwill take you back to whatever page you were on before
/
, which often isn'teven 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.