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

Non-Singleton Controller Discussion #1637

Closed
ghempton opened this issue Dec 31, 2012 · 29 comments
Closed

Non-Singleton Controller Discussion #1637

ghempton opened this issue Dec 31, 2012 · 29 comments
Milestone

Comments

@ghempton
Copy link
Member

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:

  • Bookkeep at the view layer
  • Bookkeep at the route level
  • Bookkeep at the content layer (e.g. on a per-model basis)

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
Copy link
Member

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

@ghempton
Copy link
Member Author

@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
Copy link
Member

@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
Copy link
Member Author

@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
Copy link
Member

tomdale commented Dec 31, 2012

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

@ghempton
Copy link
Member Author

@tomdale cool! Looking forward to hearing them.

@stefanpenner
Copy link
Member

👍

@wycats
Copy link
Member

wycats commented Dec 31, 2012

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
Copy link
Member

@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
Copy link
Contributor

zeppelin commented Jan 2, 2013

@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
Copy link
Member

hjdivad commented Jan 2, 2013

@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
Copy link
Contributor

zeppelin commented Jan 2, 2013

@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
Copy link
Member

hjdivad commented Jan 2, 2013

@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
Copy link
Member

hjdivad commented Jan 2, 2013

@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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Member

tchak commented Jan 3, 2013

@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
Copy link
Member

+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.

tchak pushed a commit to tchak/ember.js that referenced this issue Jan 7, 2013
```
var ControllerClass = Em.Controller.extend();
var controller = Em.ArrayController.create({ content: Em.A([0, 1]) });

controller.objectAtContent(0);               // an instance of ControllerClass
controller.objectAtContent(0).get('content') // 0
```

This partially addresses emberjs#1637, but makes no attempt at supporting reusable
views.
hjdivad added a commit to hjdivad/ember.js that referenced this issue Jan 8, 2013
```javascript
App.ItemController =
  Em.ObjectController.extend();

var arrayController = Em.ArrayController.create({
  itemControllerClass: 'ItemController',
  content: Em.A([0, 1])
});

controller.objectAtContent(0);               // an instance of ItemController
controller.objectAtContent(0).get('content') // 0
```

This partially addresses emberjs#1637, but makes no attempt at supporting reusable
views.
@sly7-7
Copy link
Contributor

sly7-7 commented Jan 16, 2013

@colymba
Copy link

colymba commented Jan 16, 2013

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

@hjdivad
Copy link
Member

hjdivad commented Jan 16, 2013

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

@colymba
Copy link

colymba commented Jan 16, 2013

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
Copy link
Member

hjdivad commented Jan 16, 2013

@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
Copy link
Member

hjdivad commented Jan 16, 2013

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

@colymba
Copy link

colymba commented Jan 16, 2013

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

@colymba
Copy link

colymba commented Jan 16, 2013

@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
Copy link
Member

hjdivad commented Jan 16, 2013

@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
Copy link

colymba commented Jan 17, 2013

@hjdivad that's what I thought. thx

@ahawkins
Copy link

@ghempton can this be closed?

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

No branches or pull requests