ember.js icon indicating copy to clipboard operation
ember.js copied to clipboard

LinkView "active" class not always applied after retried transitions

Open ehntoo opened this issue 10 years ago • 20 comments

I spotted an odd issue with the login workflow of a new app I started working on last week. After a user completed our login process, the navigation link for the route they came back to did not have an 'active' class applied.

I've reproduced the issue in a jsbin here: http://emberjs.jsbin.com/livisemixu/1/

To see the behavior, click on the "Secret" link, then the "Log in" link that appears in the route. There will be no active class when the transition is subsequently resumed. If you click back to the index and revisit the "secret" route, the class is then applied correctly.

Interestingly, if the model hook of the retried transition returns a promise that is not resolved immediately, the issue does not occur.

I've reproduced this both in 1.11.0-beta.4 and on canary, and will see if I can put together a failing test.

ehntoo avatar Mar 01 '15 19:03 ehntoo

Probably related to https://github.com/emberjs/ember.js/issues/10421

topaxi avatar Mar 01 '15 19:03 topaxi

I encountered this same problem recently. And it is most likely related to https://github.com/emberjs/ember.js/issues/5208

I'm using ember-simple-auth and it does something similar. It's caused by the previousTransition.retry();

Based on the bug report, when you retry the transition, the router state isn't updated, so the link helpers are comparing their route to the wrong route.

More Info:

The router state is set in didBeginTransition but this is not called when a transition is retried.

engwan avatar Mar 25 '15 09:03 engwan

Thanks, @engwan. The issue manifested in my app when using simple-auth as well, but I reduced it down to the testcase in the jsbin in the first post.

Based on my own investigation, I'd agree that #5208 is very likely the cause of this issue, or at least related.

ehntoo avatar Mar 25 '15 14:03 ehntoo

I spent some more time looking into this without a whole lot of progress, but have implemented a (rather ugly) workaround by setting up the default model hook behavior for generated routes to return a promise.

  Ember.Route.reopen({
    model: function() {
      return new Ember.RSVP.Promise(function(resolve) {
        Ember.run.later(resolve, 1);
      });
    }
  });

You'll also need to be aware that all model hooks you override will also need to return a promise.

ehntoo avatar Apr 09 '15 14:04 ehntoo

$100 and a Dollar Shave Club t-shirt to whomever fixes this bug. tweet to claim @pfisher42

pwfisher avatar Aug 04 '15 22:08 pwfisher

Just ran into this. Thanks @ehntoo for the workaround.

gerry3 avatar Oct 19 '15 19:10 gerry3

I'm running into this still with Ember 2.4.0.

barryofguilder avatar Mar 02 '16 15:03 barryofguilder

The newly formed "issues team" is starting to run through the backlog on issues. It'd be really helpful to get a more recent reproduction of this issue (preferably in a Twiddle) so we can work on getting it fixed.

acorncom avatar Apr 16 '16 05:04 acorncom

@acorncom - I've transcribed my jsbin from the original report into a twiddle here: https://ember-twiddle.com/5e6e66e8e1ecbea27e18d757d275899b . It appears to run without any deprecation warnings against release, beta, and even canary.

ehntoo avatar Apr 17 '16 15:04 ehntoo

That's quite helpful, yep I see it too ;-)

acorncom avatar Apr 21 '16 15:04 acorncom

Is there some trick for hotfix?

Nav4e avatar May 21 '16 10:05 Nav4e

@Nav4e My only workaround at the moment is to hardcode a transition to a specific route on login:

actions: {
  authenticate: function() {
    let credentials = this.getProperties('identification', 'password');
    this.get('session').authenticate('authenticator:custom', credentials).then(() => {
      this.set('errorMessage', null);
      this.transitionToRoute('targetRoute');
    }).catch(() => {
      this.set('errorMessage', 'Incorrect email or password. Try again.');
    });
  }
}

By calling transitionToRoute, it updates the router state.

mattcoker avatar Jun 06 '16 16:06 mattcoker

I worked around this by replacing transition.retry() with:

function * params(transition) {
  for (const info of transition.handlerInfos) {
    for (const value of Object.values(info.params)) {
      yield value;
    }
  }
}

const { url, name, queryParams } = transition.intent;
if (url) {
  this.transitionTo(url);
} else {
  this.transitionTo(name, ...params(transition), { queryParams });
}

dwickern avatar Sep 26 '16 21:09 dwickern

@dwickern you are a lifesaver, that issue has been plaguing me for a long time. That will do until there's a real fix for this issue.

Thanks!

kjhangiani avatar Aug 14 '17 03:08 kjhangiani

@dwickern actually ran into a bug with your implementation - because you used Object.values on params, (which is a keyed object), the params were coming out in an undefined order, resulting in the transition failing due to incorrect positional params

I changed it to:

function * contexts(transition) {
	for (const ctx of transition.intent.contexts) {
		yield ctx;
	}
}
const { url, name, queryParams } = transition.intent;
if (url) {
	this.transitionTo(url);
}
else {
	this.transitionTo(name, ...contexts(transition), { queryParams });
}

which fixed the issue for me

kjhangiani avatar Aug 14 '17 22:08 kjhangiani

@kjhangiani Good catch. Maybe you have a route with more than one dynamic segment like this.route('my-route', { path: ':first/:second' })

If it works then the code can probably be simplified to

let { url, name, contexts, queryParams } = transition.intent;
if (url) {
  this.transitionTo(url);
} else {
  this.transitionTo(name, ...contexts, { queryParams });
}

dwickern avatar Aug 14 '17 22:08 dwickern

@dwickern thanks, that is indeed a lot simpler. It was due to multiple dynamic segments in the same route, yes.

I have updated my code to use your solution and it seems to work as expected. Thanks!

kjhangiani avatar Sep 13 '17 22:09 kjhangiani

@Nav4e @acorncom @barryofguilder @dguettler @dwickern @ehntoo @engwan @gerry3 @hbrysiewicz @jerel @kjhangiani @mattcoker @mike-north @pwfisher @topaxi is this still an issue, perhaps we should close or create a new reproduction of this, what do you think?

pixelhandler avatar Sep 28 '18 17:09 pixelhandler

we still have the workaround https://github.com/emberjs/ember.js/issues/10557#issuecomment-91249540 with Ember 2.18.0. would like to remove it if possible. maybe someone can confirm the issue has been fixed or create a reproduction on https://ember-twiddle.com

gerry3 avatar Sep 28 '18 18:09 gerry3

This needs to be reproduced on a modern Ember >= 4.12

kategengler avatar Apr 29 '25 16:04 kategengler