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 canary] Decouple route transition from view creation #10372

Merged
merged 1 commit into from
Feb 8, 2015

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Feb 6, 2015

This closes #9814 and #10304, which are examples of a class of problems caused by
the way the router synchronously reaches into the view hierarchy to set
outlet state. It is a significant refactor of the way the router
communicates state to outlets.

What motivates this change?

  • Simple examples like {{#if something}}{{outlet}}{{/if}} are
    incorrect under the current model.
  • Richer examples like block-helpers to enable animation also
    suffer. In general, the router cannot know when and whether a
    particular outlet is going to exist, and it shouldn't need to know.
  • The router maintains a bunch of view-related state that is actually
    redundant with the view hierarchy itself, leading to unnecessary
    complexity.
  • This eliminates the longstanding weirdness & confusion caused by the
    fact that we used to create new View instances and then throw them
    away if they looked similar enough to ones that were already
    rendered. That functionality is now covered by state diffing in the
    OutletView.
  • We reduce the API surface area between router and view layer in a way
    that should make it easier to experiment with swapping in compatible
    implementations of either.
  • As a bonus, this changes makes outlets work in an observer-free way
    that will make them easy to integrate with upcoming planned view
    layer optimizations.

How does this work?

  • Rather than directly building and linking views, the router builds up
    an abstract summary of the render decisions that have been made by
    the current routes.
  • This state is cheap to recalculate as needed. It doesn't do any view
    creation. To avoid expensive observer creation & teardown, we just
    recreate the whole thing and use a setState-like mechanism to
    propagate the changes through the outlet hierarchy. This gives us
    optimal granularity of updates.
  • Actual view instantiation moves into the OutletView -- within the
    view layer where it belongs. Each outlet does a diff to see whether
    it should rerender itself or propagate inner changes down to its
    child outlets.
  • To bootstrap rendering, the router creates a single top-level outlet,
    after which all view creation is internal to the view layer.

Does this break any existing semantics?

  • No, as far as I can tell.

Could this get even better if we decided to deprecate some old
semantics?

  • Yes. It would be better if users' renderTemplate implementations on
    Routes were required to be idempotent. Then we could eliminate a
    bunch of the remaining state from them.
  • Similarly, we should consider forbiding calling render and
    disconnectOutlet at arbitrary times that are outside the scope of
    renderTemplate. It makes the programming model harder to reason
    about.
  • Also, when we deprecate the render helper we can eliminate the
    remaining use of _activeViews state tracking on the router. That is
    the only remaining use for it.

@@ -1003,6 +1004,7 @@ Application.reopenClass({

registry.injection('view', 'renderer', 'renderer:-dom');
registry.register('view:select', SelectView);
registry.register('view:outlet', OutletView);
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be prefixed with a dash, it is private and we don't want any apps having collisions with their outlet routes...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no strong feelings about this, but why is it different from all these other registrations that are not prefixed? They all seem equally subject to collisions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess because we consider the others public API that people are supposed to know about. Hmm.

Copy link
Member

Choose a reason for hiding this comment

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

I am unsure if the others are public api either. But regardless this offers a mild buffer from resolved alternatives.

Copy link
Member

Choose a reason for hiding this comment

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

My concern is that adding this here removes the ability for apps to use the word "outlet" as a route name (which I have done actually).

As far as why the others are not prefixed, I only see three other views being registered:

  • select was added intentionally as public API (to be used as {{view 'select'}}), and was therefore not prefixed.
  • view:toplevel and view:default have existed since v1.0.0-rc1 (see here). I am unsure if they are considered public API, but it is grandfathered in under SemVer (as far as naming collisions go) due to being present pre-1.0.0.

Copy link
Member

Choose a reason for hiding this comment

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

If your position is that this is actually a public API (for example if folks are supposed to put a file at app/views/outlet.js that exports a custom OutletView), I suppose that is a different argument, but I do not think that is what you are suggesting...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, that's fine, I will add the dash.

@rwjblue
Copy link
Member

rwjblue commented Feb 6, 2015

This looks wonderful!

@quaertym
Copy link
Contributor

quaertym commented Feb 7, 2015

👍

1 similar comment
@jcope2013
Copy link
Contributor

👍

@ef4
Copy link
Contributor Author

ef4 commented Feb 7, 2015

@rwjblue has pointed out that this will let us remove the isTop flag that gets added by the template compiler.

And @mmun pointed out that this eliminates our last internal use of ContainerView's currentView, giving us the option of deprecating it.

This closes emberjs#9814 and closes emberjs#10304, which are examples of a class of
problems caused by the way the router synchronously reaches into the
view hierarchy to set outlet state. It is a significant refactor of the
way the router communicates state to outlets.

What motivates this change?

 - Simple examples like `{{#if something}}{{outlet}}{{/if}}` are
   incorrect under the current model.

 - Richer examples like block-helpers to enable animation also
   suffer. In general, the router cannot know when and whether a
   particular outlet is going to exist, and it shouldn't need to know.

 - The router maintains a bunch of view-related state that is actually
   redundant with the view hierarchy itself, leading to unnecessary
   complexity.

 - This eliminates the longstanding weirdness & confusion caused by the
   fact that we used to create new `View` instances and then throw them
   away if they looked similar enough to ones that were already
   rendered. That functionality is now covered by state diffing in the
   `OutletView`.

 - We reduce the API surface area between router and view layer in a way
   that should make it easier to experiment with swapping in compatible
   implementations of either.

 - As a bonus, this changes makes outlets work in an observer-free way
   that will make them easy to integrate with upcoming planned view
   layer optimizations.

How does this work?

 - Rather than directly building and linking views, the router builds up
   an abstract summary of the render decisions that have been made by
   the current routes.

 - This state is cheap to recalculate as needed. It doesn't do any view
   creation. To avoid expensive observer creation & teardown, we just
   recreate the whole thing and use a `setState`-like mechanism to
   propagate the changes through the outlet hierarchy. This gives us
   optimal granularity of updates.

 - Actual view instantiation moves into the OutletView -- within the
   view layer where it belongs. Each outlet does a diff to see whether
   it should rerender itself or propagate inner changes down to its
   child outlets.

 - To bootstrap rendering, the router creates a single top-level outlet,
   after which all view creation is internal to the view layer.

Does this break any existing semantics?

 - No, as far as I can tell.

Could this get even better if we decided to deprecate some old
semantics?

 - Yes. It would be better if users` `renderTemplate` implementations on
   `Route`s were required to be idempotent. Then we could eliminate a
   bunch of the remaining state from them.

 - Also, when we deprecate the `render` helper we can eliminate the
   remaining use of `_activeViews` state tracking on the router. That is
   the only remaining use for it.
ef4 added a commit that referenced this pull request Feb 8, 2015
[BUGFIX canary] Decouple route transition from view creation
@ef4 ef4 merged commit b186e6e into emberjs:master Feb 8, 2015
@mmun
Copy link
Member

mmun commented Feb 8, 2015

👏

@rwjblue
Copy link
Member

rwjblue commented Feb 8, 2015

I accidentally branched for the 1.11 betas before this landed. Pulled in for 1.11.0-beta.2.

var target;
var myState = {
render: renderOptions,
outlets: Object.create(null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this needs to be Ember.create for ie8 et al support

Copy link
Member

Choose a reason for hiding this comment

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

ya cc @ef4

Copy link
Member

Choose a reason for hiding this comment

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

@stefanpenner - It was already fixed in a followup PR.

Copy link
Member

Choose a reason for hiding this comment

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

👍

ef4 added a commit to ef4/ember.js that referenced this pull request Feb 8, 2015
@machty pointed out that emberjs#10372 uses an ES5-only feature.
@tim-evans
Copy link
Contributor

I'm using canary, and it seems like this refactoring broke my app.

this.connections.push(renderOptions);

this.connections is undefined. Are there other deprecations that come with this change that should be communicated?

@rwjblue
Copy link
Member

rwjblue commented Feb 9, 2015

@tim-evans - Can you open an issue with a repro so we can track this down?

@tim-evans
Copy link
Contributor

@rwjblue I'll try to do this, but I'm currently in a big upgrade from 1.8 to 1.11. I'm just flagging this as an issue for now.

@rwjblue
Copy link
Member

rwjblue commented Feb 9, 2015

@tim-evans - Gotcha, good luck!

@machty
Copy link
Contributor

machty commented Feb 9, 2015

@tim-evans I'd be on the lookout for a this.render() in an unusual place (e.g. outside of renderTemplate or action handlers), but I could be wrong.

@tim-evans
Copy link
Contributor

@machty just checked around that, but it doesn't seem to be it at first glance.

ef4 added a commit that referenced this pull request Feb 15, 2015
@machty pointed out that #10372 uses an ES5-only feature.
@kamal
Copy link
Contributor

kamal commented Mar 3, 2015

One use case that broke for me is calling this.render without specifying the into option. Somehow Ember used to be able to look up the chain of templates to find the first named outlet?

This used to work:

renderTemplate: function(){
  this._super();
  this.render('settings/_nav', {
    outlet: 'subnav'
  });
}

@mixonic
Copy link
Member

mixonic commented Mar 8, 2015

@kamal that sounds like a regression. I suggest opening a ticket (ideally with a reproduction) to track a fix.

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.

Tries to reuse a destroyed view when rerendering an outlet
10 participants