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] Retry transition created with replaceWith causes a history replacement #15168

Merged
merged 1 commit into from
Apr 25, 2017

Conversation

cibernox
Copy link
Contributor

This issue was fixed upstream in router.js, I'm just repeating the information here
for context.
Before this change the next code:

let transition = this.replaceWith('new.route')
transition.abort()
transition.retry();

caused the app to transition to 'new.route' without replacing the previous history entry.
After this change, Transition#retry respects the intention of the original transition. In
the snippet above 'new.route' will now replace the previous route in the history.

…eplacement

This issue was fixed upstream in router.js, I'm just repeating the information here
for context.
Before this change the next code:

```js
let transition = this.replaceWith('new.route')
transition.abort()
transition.retry();
```

caused the app to transition to `'new.route'` without replacing the previous history entry.
After this change, `Transition#retry` respects the intention of the original transition. In
the snippet above `'new.route'` will now replace the previous route in the history.
@rwjblue rwjblue merged commit aa1d66e into emberjs:master Apr 25, 2017
@cibernox cibernox deleted the bum-router-js branch April 25, 2017 20:56
@workmanw
Copy link

Ember try alerted us to some test failures with ember-2.14.0-beta.1 and I bisected them down to this commit. I tried to create a twiddle, but it seems that I cannot do so with beta builds at the moment.

The test failure is simply redirecting an unauthenticated user to the login page, then calling transition.retry after login. The transition is successfully redirected to the right route, but it leaves the URL in the wrong state.

Should I open a new issue for this?

@cibernox
Copy link
Contributor Author

@workmanw I'm interested. Is the transition made with replaceWith? Can you share an example?

@workmanw
Copy link

workmanw commented Apr 28, 2017

@cibernox I guess I could probably make an example ember app in lieu of a twiddle.

Here are some examples:

Saving the retry transition and redirecting

// app/routes/authenticated.js
// Everything inside this route requires the user to be authenticated
export default Ember.route.extend({
  afterModel(model, transition) {
    let authService = this.get('auth');
    if (!authService.get('isLoggedIn')) {
      authService.set('retryTransition', transition);
      this.transitionTo('login');
    }
  }
});

User login

// app/services/auth.js
export default Ember.Service.extend({
  login(user, password) {
    this.get('ajax').post('/login', { user, password }).then(() => {
      let retryTransition = this.get('retryTransition');
      if (retryTransition) {
        retryTransition.retry();
        this.set('retryTransition', null);
      } else {
        this.get('routerServiceStub').transitionTo('authenticated.home');
      }
    });
  }
});

So you can see nothing is too out of the ordinary as far as I can tell. What happens is the user lands on the right route after a successful login, but the URL is still for the login page. Also, we're using the hash router.

EDIT: Let me make an app to reproduce this. I should have done so before submitting this comment.

@rwjblue
Copy link
Member

rwjblue commented Apr 28, 2017

@workmanw - Thanks for alerting us! Can you create a new issue once you have a repro?

@workmanw
Copy link

@rwjblue Roger that. I just created a failing repo. I have to jump for a few, but you can expect a new issue later on. Thanks!

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.

None yet

4 participants