emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.45k stars 4.21k forks source link

routing to different handler from same link-to #4391

Closed dagda1 closed 10 years ago

dagda1 commented 10 years ago

I am not sure if this is connected to 1.4.0 or it has been there before or if this is my misunderstanding of how the router works but it would be good for me to get some clarity on this situation.

I have this jsbin that illustrates the problem.

Steps to reproduce:

  1. Click on the companies link in the top nav - Everything works as expected, the correct handler with a route name of companies is selected and its afterHook is called.
  2. If I am on the companies route and then I click on the companies link again on the top nav again then a handler with a routeName of companies.index is executed which is not a route that I have defined and therefore has no overrriden afterModel hook and I do not get the behaviour I expect. I have to override willTransition to stop the transition from the same route. I would have expected the same handler to get executed each time.

I find this strange that clicking on a link that is generated from the same link-to link results in a different handler being executed in different circumstances.

machty commented 10 years ago

@dagda1 all this.resource's have an implicit .index route, even if you don't explicitly define one. A link-to companies ultimately translates to link-to companies.index. We fixed a bug in 1.4.0 which was causing the model hooks on routes that didn't need to be re-evaluated in any way (e.g. if you transition from a.b to a.c, then a's model hooks shouldn't be run again to avoid wasteful computation / server queries), and I think it's affecting you in this case; if you're in companies.whatever, transitioning to any sibling routes aren't going to refire any logic on CompaniesRoute model hooks because that route hasn't been invalidated in any way. What you want is to define redirect logic on your CompaniesIndexRoute (which would have always worked in the past

dagda1 commented 10 years ago

How do you feel about it linking as expected? I think it is confusing and even more confusing to have to add a redirect hook for what does not feel like an edge case or exception. I just want to link to my routes and not worry about it. I fear I might have to do this in a number of places and it feels unnecessary.

dagda1 commented 10 years ago

At this stage, I would prefer wasteful computation over strange behaviour. You would just expect these links to just work.

machty commented 10 years ago

So, you can link to your routes and not worry about it; just move your redirect hook to CompaniesIndexRoute. It's the same amount of code, no?

machty commented 10 years ago

redirect and afterModel in this case are functionally equivalent. You have it as afterModel now, so I just mean move afterModel to CompaniesIndexRoute.

dagda1 commented 10 years ago

@machty I can work around my problems in multiple ways so that is not the immediate concern.

I am more thinking about the long term and to not have surprising behaviour for things that seem like they should just work.

dagda1 commented 10 years ago

It is that or add some assertions to alert the user that they are going down the wrong path,

machty commented 10 years ago

I'm not sure how the behavior you're describing is expected... if you have a nested route hierarchy of foo.bar.baz.bat, and you have a link-to foo (and of course foo is a resource), why is it expected that that should invoke model hooks on FooRoute?

dagda1 commented 10 years ago

I would expect a link-to foo to behave the same way in any part of the application and not have to have code in different places for different situations.

dagda1 commented 10 years ago

It certainly struck me as unexpected or I would not have raised the issue

machty commented 10 years ago

I really think what's missing here is that we didn't catch this as a breaking change to make a lot of noise about upon the release for 1.4.0. I don't think there's any sort of assertions to add in this case because the way you have your code actually does serve a well-defined purpose, which is that transitions into the companies hierarchy must pass whatever validation/redirection you put in CompaniesRoute model hooks. Your case is different: you expect links from within a hierarchy to refire hooks that involve entry into that route, which doesn't make conceptual sense.

I would expect a link-to foo to behave the same way in any part of the application and not have to have code in different places for different situations.

The "same way" is not the goal so much as "well-defined" is the goal when hierarchical structures are involved. Given that the data required to perform/complete a transition is different depending on your app state, as well as where you're transitioning to/from, not all links are going to behave the same way depending on the present context.

I want to hear your suggestions for making things more straightforward but I need to make sure we're on the same conceptual page as to why things are the way they are. At the very least, I think we can agree that this should have been loudly mentioned as a potential breaking change upon the 1.4.0 release.

dagda1 commented 10 years ago

In hindsight yes, it would have been good to have been aware of it.

But the problem remains, I want to link-to or transitionTo the CompaniesRoute and have everything displayed then I can't if I am in a child route. But I cannot after the change.

If I create a CompaniesIndexRoute and put the afterModel hook there then an undefined model will be passed to the hook which is of no use. I could use `controllerForormodelFor`` but this feels like I am working round the problem or at least it does not feel natural.

machty commented 10 years ago

@dagda1 the good news is that on beta/canary, there's a feature where routes that don't specify their own model inherit the model from their parent resource, so when that's released in 5 weeks you'll be able to just have

App.CompaniesIndexRoute = Ember.Route.extend({
  redirect: function(model) { // or afterModel
    // redirect logic
  }
});

Until then you'll need to use modelFor, which is not as succinct as what you have, but it's not the kind of un-succinct that means "code smell"

dagda1 commented 10 years ago

THat certainly sounds better although, I think I'll be waiting a while before my next upgrade :)

machty commented 10 years ago

@dagda1 yeah, I feel ya. Sorry this one was so rough, talking with others to see if/how we can prevent this sort of thing. There was a lot of undefined behavior in router.js and other microlibs that was ironed out in 1.4.0 and you're unfortunately feel the pain of the ill-defined/undefined behavior solidifying in a direction that broke your app.

Let me know if the CompaniesIndexRoute approach with modelFor ends up working for you in the meantime.

ebryn commented 10 years ago

@dagda1 This is unfortunately a circumstance where you were relying on buggy behavior.

I don't want you to be afraid of upgrades. If you're not upgrading your app within a week of a release, one of us is "doing it wrong".

dagda1 commented 10 years ago

@machty the hook works with `modelFor. The funny thing is, I just realised that I have a big work around to counteract the model hooks always firing :).

machty commented 10 years ago

@dagda1 oh please tell me that you can get rid of that workaround. That would be the silver lining

dagda1 commented 10 years ago

I can, all is well that ends well.

Thanks for your input.

Cheers

Paul Cowan

Cutting-Edge Solutions (Scotland)

blog: http://thesoftwaresimpleton.com/ website: http://www.cuttingedgesolutionsscotland.com/

On 18 February 2014 15:17, Alex Matchneer notifications@github.com wrote:

@dagda1 https://github.com/dagda1 oh please tell me that you can get rid of that workaround. That would be the silver lining

Reply to this email directly or view it on GitHubhttps://github.com/emberjs/ember.js/issues/4391#issuecomment-35393835 .

machty commented 10 years ago

Woooo hoooo! If only I could close already-closed issues.