Prevent unnecessary re-rendering when only route context has changed #2563
Conversation
|
@ef4 can you add a regression test. |
|
Sure. |
|
seems to be failing. |
|
|
||
| Ember.run(function() { | ||
| router.handleURL("/page/second"); | ||
| }) |
There was a problem hiding this comment.
Add a ; here and on line 1798 and the build should pass
|
👍 This would be extremely helpful. Would provide a solution to my StackOverflow question Avoid current route re-enter (and re-render) on url dynamic segment update |
|
Freaking semicolons... Sorry, I'm spoiled by mostly writing CoffeeScript, and I could swear I had run the whole test suite. Should be fixed now. I squashed the minor edits. |
|
understood bro. FYI our ci runs seems to no longer merge cleanly. (been a busy ember day) |
It should only be necessary to run `renderTemplate` when a route is entered. Subsequent changes to a route's context Just Work(tm) thanks to the usual magic of bindings. In addition to a theoretical performance benefit, this change gives the controller more control over precisely when and how the view will transition from model-to-model. (In my case, I'm using this capability to nicely animate the transition.) This change doesn't break any unit tests or change the API. But if someone is doing something weird in `renderTemplate` (like choosing their template based on the model, or doing something else with side-effects), they will get breakage. I think those cases are sufficiently pathological not to worry about.
|
Rebased and resolved conflicts. |
Prevent unnecessary re-rendering when only route context has changed
|
@ef4 thanks dude! |
|
Made my friday, thanks! |
|
@robharper happy friday! |
|
@ef4 could you make the proper PR to https://github.com/tildeio/router.js for the change you did to |
|
@stefanpenner Sure, no problem, but I want to reconsider this PR (sorry I didn't see it earlier). I don't think router.js should know anything about |
|
@machty It is possible to solve this issue without changing router.js, but I think that solution is more fragile. Currently, the router knows about three hooks on each route: We want the route to render its template only when we're entering for the first time, but after Now, it's possible to make the route more stateful instead: it can set a flag when The router is the part of the code that reliably knows whether we're just updating context or entering. It should be responsible for firing the right hook that conveys that information directly. My change creates a fourth hook and calls it But there are even better ways to redesign the API. If I was doing it from scratch: Given these, each route change would fire only one hook, making the intent very clear to the receiving route. The route is still free to implement |
|
As it stands, the existing API is not good enough, because |
|
@ef4 we're doing some substantial router refactoring that will address many of these issues you've brought up. I'll definitely keep you posted on this stuff as they come out. But in the meantime we want to keep router.js separate from any knowledge of rendering templates and so on, even if a more Ember-based approach seems more fragile. I think something in Ember.Route that checks for a duplicate render would make sense and I think we'll be moving this logic there. Would you want to take a stab at that? @stefanpenner lemme know if you agree |
|
"But in the meantime we want to keep router.js separate from any knowledge of rendering templates and so on" But it is separate, and there's still no knowledge of template rendering in router.js. All we're really doing here is creating a new hook, and now you're really making me wish I named it something different, because the name is confusing the issue. Call it "finishedEntering" instead. "In the meantime", multiple people are running up against this issue, and we have a working solution that does not complicate the router or make it any harder to refactor. |
|
@ef4 let's keep this PR in the meantime. I agree with you, I just want things to be in the right place. Taking a look at the code now and I there's a more sensible solution with |
|
I'm not sure I understand what you mean.
|
|
Sorry, @ef4, I'm making the mistake I've chided others for in that I'm putting in the minimal effort to point out something is wrong without taking the proper time to fully understand the problem to provide a better solution; I'll try to be more on the ball from now on. What if we just made the |
|
Making
|
|
@ef4, thanks. For what it's worth, there's been some discussion about idempotent rendering as a possible way to facilitate state-based animations, e.g. if you have routes A and B, and you'd like some UI element in B to slide in (but you don't want to render it in A), you could feasibly have both B and transition state |
|
Yeah, animation is the main reason I opened this pull request in the first place. As written, it allows you to animate context changes within the same route, and I'd be happy to see a solution for cross-route animation as well. But idempotent render doesn't fully solve the problem. We would also need to make |
This change prevents us from tearing down and recreating a view when the new view is functionally identical to the old view. This has important benefits: - when you're changing context within a route, the view itself remains stable, and the controller has full control over how and when to update the data it's presenting to the view layer. (Previously, you got a forced re-render immediately whether you were ready for it or not.) - when you're changing between routes, you can decide to keep the prior view in place, and replace it later when you're ready. (Think animated transitions between routes.) This supercedes the proposed changes in PR emberjs#2563 (which has already been reverted).
It should only be necessary to run
renderTemplatewhen a route isentered. Subsequent changes to a route's context Just Work(tm) thanks
to the usual magic of bindings.
In addition to a theoretical performance benefit, this change gives
the controller more control over precisely when and how the view will
transition from model-to-model. (In my case, I'm using this capability
to nicely animate the transition.)
This change doesn't break any unit tests or change the API. But if
someone is doing something weird in
renderTemplate(like choosingtheir template based on the model, or doing something else with
side-effects), they will get breakage. I think those cases are
sufficiently pathological not to worry about.