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

Non-Singleton Controller Discussion #1637

Closed ghempton closed 11 years ago

ghempton commented 11 years ago

I just want to open up a discussion around how this could possibly be implemented and what the api would look like (especially in light of the new injection container and router).

The canonical use case for this is in conjunction with an {{each}} helper that requires an ObjectController for each item.

As it stands, I can think of two ways to implement this:

  1. Use a custom Ember.ArrayProxy subclass which wraps each item in its content in a controller. This subclass intercepts array change events to handle bookkeeping of the controllers. (This is the method I currently use in my apps.)
  2. Use the injection container with non-singleton controllers. Simply register the controller in the container as a non-singleton and do lookup as normal. The open problem here is where and when to cleanup these controllers.

With option 2 in mind, consider the following handlebars snippet:

{{#each item in this}}
  {{render 'item' item}}
{{/each}}

This assumes the existence of the {{render}} helper (see #1628) and I also think it would be good to eventually give {{each}} render semantics, but this snippet is helpful for discussion.

If a controller is registered as a non-singleton with the container, then this snippet should hypothetically function as expected. In fact, this has the added benefit that things like {{render}} could be entirely agnostic as to whether there is a singleton or non-singleton controller backing it. The only problem is that these non-singleton controllers will never be destroyed.

Therefore, in order for option 2 to be viable, there needs to be some mechanism to track these controllers and clean them up when appropriate. Three possibilities that immediately come to mind:

The view layer is a poor option due to the fact that the controller would be destroyed during re-render.

The route level could be plausible but seems unnecessarily tied to the routing system.

Perhaps, book-keeping on a per-model basis is the best option? The render semantics could be such that for non-singleton controllers, a listener is created that destroys the controller at the same time the model is destroyed? Once ember-data has some sort of store/cache clearing mechanism I can see this making a lot of sense. This is also in some ways very similar to using an ArrayProxy.

Thoughts?

stefanpenner commented 11 years ago

Something related and worth skimming over may be guice's injection scopes: http://code.google.com/p/google-guice/wiki/Scopes

ghempton commented 11 years ago

@stefanpenner we essentially already have scopes by having the singleton option. The problem, as I see it, is when to call destroy on the non-singleton scoped controllers.

stefanpenner commented 11 years ago

@ghempton wouldn't a scope bound to the lifecycle of an object be like a good fit?

Singleton could be viewed as a scope bound to the lifecycle of the entire application.

ghempton commented 11 years ago

@stefanpenner makes sense. If we opted to go with non-singleton controllers that have life-cycles tied to other objects, then I could picture this being implemented as some sort of scope.

tomdale commented 11 years ago

We literally just had an hour-long discussion about this after lunch today. We can try to write up some of our thoughts.

ghempton commented 11 years ago

@tomdale cool! Looking forward to hearing them.

stefanpenner commented 11 years ago

:+1:

wycats commented 11 years ago

Thoughts on Controllers

Why Do We Want This?

Remember that models store persistent state and controllers store application state.

Application state is state that should be saved for the entire runtime of the application; persistent state should be saved even if the user closes the app and later re-opens it.

Controllers also present themselves as models to views, augmenting the model with properties that are application state—i.e., they should survive the destruction of a particular view.

Let's use a simple motivating example involving an expanded state (there is also the case of reusable views, but we'll discuss that separately below).

<!-- post.handlebars -->
<div class="post">
{{#if isExpanded}}
  <h1>{{title}}</h1>
  <div>{{body}}</div>
  <button {{action "less"}}>Contract post</button>
{{else}}
  <h1>{{title}} <button {{action "more"}}>Show more...</button></h1>
{{/if}}
</div>

We would have a controller like this:

App.PostController = Ember.ObjectController.extend({
  isExpanded: false,

  more: function() {
    this.set('isExpanded', true);
  },

  less: function() {
    this.set('isExpanded', false);
  }
});

In this example, even if the template was torn down and re-rendered, the expanded state of this post would be preserved.

In general, using controllers for state ensures that the state will not be coupled to the life-cycle of the view. In Backbone applications, people store state like this on non-persistable models. In Ember, we call these objects controllers.

Unfortunately, like Punxsutawney Phil on Groundhog's Day, a problem pops up when you try to apply a similar strategy inside a {{#each}} helper:

<!-- posts.handlebars -->
{{#each post in posts}}
  <div class="post">
  {{#if isExpanded}}
    <h1>{{post.title}}</h1>
    <div>{{post.body}}</div>
    <button {{action "less"}}>Contract post</button>
  {{else}}
    <h1>{{post.title}} <button {{action "more"}}>Show more...</button></h1>
  {{/if}}
  </div>
{{/each}}

In this case, the current controller is the posts controller, so expanding a single post would invoke the more action on the controller, and the isExpanded property is shared across all the elements in the loop.

To solve this problem today, we have a few bad options.

  1. Save the isExpanded property on the model, treating the model like a controller. This is an easy solution, but if you want to use the same model in multiple contexts, you have to namespace your expanded properties, keeping track of what ends up being a junk drawer of application state in the wrong place.
  2. Define a new view class and store the isExpanded property there. This approach is good if you want the expanded state to reset every time you leave and re-enter the current route, because isExpanded is tied to the view lifecycle. In most cases, however, this would produce a subpar user experience.
  3. Override objectAtContent in your PostsController and try to wrap each new object in a controller. This approach would work, but getting it exactly right (avoiding memory leaks, invalidating caches when the underlying array changes, etc.) is tricky, and the realm of framework code, not application code.

Proposed Solution

@ghempton's proposed Option 1 addresses this issue for the common case where you want the state to persist across navigation changes.

In short, the framework would supply a itemControllerClass property on Ember.ArrayController. If this property was set, the ArrayController would automatically wrap new contents in the specified class.

Whenever the underlying Array changes (e.g. because the model was destroyed), the associated ObjectController would be thrown away via a framework-provided mechanism.

In short, you would get all of the benefits of "Bad Option 3" above, without the pitfalls of implementing it yourself.

Reusable Views

Reusable views have a different problem. In particular, they really want a view-specified controller class that wraps the model or controller provided by the parent template.

For example, imagine a calendar view, used thusly:

{{view UI.Calendar dateBinding="post.startDate"}}

By default, you can imagine the UI framework providing a default controller that the UI.Calendar view expects date to be wrapped in.

The framework may ask you to subclass this controller in order to implement certain hooks (e.g. which date ranges are greyed out), but there is currently no way to provide a controller class to a view.

Currently, the controller to which the template is bound would be required to know how many instances of UI.Calendar were in its template and create named instances of the controller subclass for each widget.

Instead, we propose:

{{view UI.Calendar dateBinding="post.startDate" controllerClass="App.CalendarController"}}

This would create an instance of App.CalendarController for each UI.Calendar widget, which would be tied to the lifecycle of the widget.

stefanpenner commented 11 years ago

@wycats thanks for the well thought out reply.

Seems like a good fit to me, but what are the thoughts on instantiating or resolving these widget controllers ala the container?

Example:

{{view UI.Calendar dateBinding="post.startDate" controllerClass="controller:calendar"}}
zeppelin commented 11 years ago

@wycats itemControllerClass is perfect!

Especially if I get the opportunity to do some introspection on each model instance to decide on what controller class should I use for that particular model, rather than having just one class for every item in the array :)

hjdivad commented 11 years ago

@zeppelin can you say more about this? What's the use case where you want something more complicated?

I'm trying to find time to work on the itemControllerClass feature separately from the reusable views feature and I'm not currently testing varying controller classes, although it likely works simply because the class is retrieved via Ember.get.

zeppelin commented 11 years ago

@hjdivad I was thinking sg similar to this:

If I want uniform itemControllerClasses for every item in the array, I just declare that:

App.MyArrayController = Ember.ArrayController.extend({
  itemControllerClass: 'App.MyItemControllerClass'
});

However, if I have an array of objects of different types, I should be able to do something like

App.MyArrayController = Ember.ArrayController.extend({
  itemControllerClass: function(model) {
    if (model.get('isSpecial')) {
      return 'App.MySpecialItemControllerClass';
    } else {
      return 'App.MyItemControllerClass';
    }
  }
});
hjdivad commented 11 years ago

@zeppelin seems fine, provided we update your example to make itemControllerClass a property

App.MyArrayController = Ember.ArrayController.extend({
  itemControllerClass: function(model) {
    if (model.get('isSpecial')) {
      return 'App.MySpecialItemControllerClass';
    } else {
      return 'App.MyItemControllerClass';
    }
  }.property()
});
hjdivad commented 11 years ago

@zeppelin I suppose it's obvious, but one concern with making itemControllerClass a property will be that bad things may happen if it returns a different value when the underlying object hasn't changed.

In particular, at the moment I'm expecting objectAtContent(idx) to return the same controller instance for the same underlying object. Obviously that can't be guaranteed if the controller class changes between multiple invocations of objectAtContent.

Any thoughts about this? What would you expect to happen in this case? Do you see a problem with making this an error case?

mikegrassotti commented 11 years ago

@wycats itemControllerClass seems a great solution. FWIW I can find in our app examples of "bad option" 1, 2, and 3. Option 3 seems by far the most elegant but like you said it's hard to get right. As a result we seem to have favored option 2, with subpar results. Having a solid solution built into the framework would be a huge win.

krisselden commented 11 years ago

I'm a fan of the itemControllerClass on an ArrayController, that tracked the lifecycle of the item in the array.

As for the widget, I think it is pointless to have the controller match the lifecycle of the widget, if it stored any state it would be reset on a parent rerender, since it is bound to post.startDate it should track the lifecycle of its 'parent' controller's content: post.

In fact that is sort of the theme here, that you have a 'child' controller that is the lifecycle of the parent's content.

tchak commented 11 years ago

@ghempton given current implementation, {{render}} helper will not work inside of a {{each}} helper because {{render}} for now assumes that you can not render the same template/controller more then once simultaneously. Because it do not make sens with singleton controllers, and because it register rendered views as active views, so you can have {{outlet}} helpers inside rendered template. I think we need a way to distinguish between singleton and non singleton controllers, so render helper can detect what kind of controller it deal with and let you render multiple times the same template with different controllers. Not sure how we can handle the activeView thing in this case. What is a semantics for {{outlet}} inside an {{each}}?

darthdeus commented 11 years ago

+1 for controllerClass on a view. I spent the last couple of days trying to make some of my views work like reusable widgets, but I end up having to put most of the logic in the view itself, even thought it doesn't belong there.

sly7-7 commented 11 years ago

What would be the workaround for this question ATM ? http://stackoverflow.com/questions/14354305/what-is-the-recommended-way-to-use-a-shared-view-in-a-template-which-needs-a-con

colymba commented 11 years ago

the itemControllerClass would be great... and save me a lot of trouble in the app am working on...

hjdivad commented 11 years ago

@colymba itemControllerClass is in master. see #1664 and http://jsfiddle.net/hjdivad/VWLaU/

colymba commented 11 years ago

thanks @hjdivad works great. Quick question though... seems to only work with this syntax {{#each controller}} but not {{#each product in controller}} or am I missing something?

hjdivad commented 11 years ago

@colymba seems bad. I will look into it and write a patch if I can reproduce the issue, and if not bug you for a jsfiddle.

hjdivad commented 11 years ago

@colymba although having said that, if you already have a jsfiddle handy, that would be great. :smile:

colymba commented 11 years ago

@hjdivad haven't got one... but will copy over some of the code am working on...

colymba commented 11 years ago

@hjdivad can't reproduce in jsfiddle anymore http://jsfiddle.net/colymba/geJ79/21/ probably had a typo... just happens when accessing content directly instead of through controller...

hjdivad commented 11 years ago

@colymba Ahh yeah, iterating over content won't work, although that has nothing to do with itemControllerClass. Iterating directly over the content of any ArrayProxy will sidestep whatever objectAt logic it employs.

colymba commented 11 years ago

@hjdivad that's what I thought. thx

ahawkins commented 11 years ago

@ghempton can this be closed?