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.

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

[BUGFIX beta] Check state of router during transition #14223

Closed

Conversation

yawboakye
Copy link

Currently, during transitions, the state of the router is not taken into
consideration. This change makes sure that both transitionTo() and
replaceWith() examine the state of the router and uses that information
to choose the appropriate method.

If there has never been a transition since the app loaded, replaceWith()
will be used. If there's been a transition already transitionTo() will
be used instead. Regardless of which method the developer used.

Special thanks to @alexspeller. In so many ways this work belongs to him.

Currently, during transitions, the state of the router is not taken into
consideration. This change makes sure that both `transitionTo()` and
`replaceWith()` examine the state of the router and uses that information
to choose the appropriate method.

If there has never been a transition since the app loaded, `replaceWith()`
will be used. If there's been a transition already `transitionTo()` will
be used instead. Regardless of which method the  developer used.
if (router.router.oldState) {
return router.transitionTo(...prefixRouteNameArg(this, arguments));
}
return router.replaceWith(...prefixRouteNameArg(this, arguments));
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer the logic be swapped around here so that the "default" thing we do is transitionTo from Route#transitionTo.

@rwjblue
Copy link
Member

rwjblue commented Sep 7, 2016

Thanks for working on this!

A few things that we need to do:

  • Tests for the various states:
    • transitionTo during initial boot (likely from route:index)
    • replaceWith during initial boot (likely from route:index)
    • transitionTo outside of initial render, during another transition (from say route:index -> route:posts do a redirect to route:posts/comments)
    • replaceWith outside of initial render, during another transition (from say route:index -> route:posts do a redirect to route:posts/comments)
    • transitionTo outside of initial render, outside of an existing transition (from an action)
    • replaceWith outside of initial render, outside of an existing transition (from an action)
  • Fix inline comment RE: transitionTo's "default"

@alexspeller
Copy link
Contributor

There are a couple of issues with this:

  1. You are having the same check in both methods – you are checking for oldState both times and calling transitionTo - the checks should be inversed.
    a. If you are calling transitionTo, and oldState is not present, then replaceWith must be called.
    b. If you are calling replaceWith, and oldState is present, then transitionTo must be called.
  2. These checks must only apply in the validation phase of a transition. Currently they apply all the time, which means that you'll break replaceWith and transitionTo in use cases outside of the validation phase of a transition

@alexspeller
Copy link
Contributor

as an architectural issue, I wonder if these fixes shouldn't actually be in router.js rather than just for ember - AFAIK the bug is actually at the lower level in the router library

@alexspeller
Copy link
Contributor

PR filed to router tildeio/router.js#197

@rwjblue
Copy link
Member

rwjblue commented Oct 31, 2016

Will be fixed by tildeio/router.js#197, closing this one...

@rwjblue rwjblue closed this Oct 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants