`ArrayController` always returns the same instance when using `itemController` #2009

Closed
josepjaume opened this Issue Feb 7, 2013 · 23 comments

Comments

Projects
None yet
6 participants
@josepjaume
Contributor

josepjaume commented Feb 7, 2013

Here's a fiddle:

http://jsfiddle.net/josepjaume/sSdHm/47/

Steps to reproduce:
Click on an element and go back. You'll see the controller is always returning the last instance.

@josepjaume

This comment has been minimized.

Show comment
Hide comment
@josepjaume

josepjaume Feb 7, 2013

Contributor

Just cleaned up the example a little bit

Contributor

josepjaume commented Feb 7, 2013

Just cleaned up the example a little bit

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Feb 8, 2013

Member

@josepjaume, looks like you have a naming conflict here. App.ThingController is used both in your array and by your thing route. I don't see any bug here.

Member

wagenet commented Feb 8, 2013

@josepjaume, looks like you have a naming conflict here. App.ThingController is used both in your array and by your thing route. I don't see any bug here.

@wagenet wagenet closed this Feb 8, 2013

@caligo-mentis

This comment has been minimized.

Show comment
Hide comment
@caligo-mentis

caligo-mentis Feb 9, 2013

Contributor

We should warn about this conflict somehow. Once I had spent about 30 minutes to reveal this mistake.

Contributor

caligo-mentis commented Feb 9, 2013

We should warn about this conflict somehow. Once I had spent about 30 minutes to reveal this mistake.

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Feb 9, 2013

Member

I'm not sure of the best way to resolve this issue directly though we have
some stuff in the pipe for helping to isolate controllers in specific
sections of your app. Hopefully that helps.

-Peter

On Feb 9, 2013, at 1:31 AM, Andrey Ivashchenko notifications@github.com
wrote:

We should warn about this conflict somehow. Once I had spent about 30
minutes to reveal this mistake.


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

Member

wagenet commented Feb 9, 2013

I'm not sure of the best way to resolve this issue directly though we have
some stuff in the pipe for helping to isolate controllers in specific
sections of your app. Hopefully that helps.

-Peter

On Feb 9, 2013, at 1:31 AM, Andrey Ivashchenko notifications@github.com
wrote:

We should warn about this conflict somehow. Once I had spent about 30
minutes to reveal this mistake.


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

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Feb 9, 2013

Member

Now that I think about this more, this may still be a bug. I'll reopen for now.

Member

wagenet commented Feb 9, 2013

Now that I think about this more, this may still be a bug. I'll reopen for now.

@wagenet wagenet reopened this Feb 9, 2013

@josepjaume

This comment has been minimized.

Show comment
Hide comment
@josepjaume

josepjaume Feb 9, 2013

Contributor

Hi @wagenet,

I think it makes sense to use the same controller as your "show" route. So either this should be filed as bug or itemController needs further explanation because I didn't get the point :)

For example, I can clearly see the case where I want to store some application state in a particular thingController (let's say collapsed/expanded) that can be set from different parts of the app - in a list or in its own "show" route.

Thanks!

Contributor

josepjaume commented Feb 9, 2013

Hi @wagenet,

I think it makes sense to use the same controller as your "show" route. So either this should be filed as bug or itemController needs further explanation because I didn't get the point :)

For example, I can clearly see the case where I want to store some application state in a particular thingController (let's say collapsed/expanded) that can be set from different parts of the app - in a list or in its own "show" route.

Thanks!

@josepjaume

This comment has been minimized.

Show comment
Hide comment
@josepjaume

josepjaume Feb 11, 2013

Contributor

My case scenario is the following:

I want to reuse the same instance of the controller on the list or in its "show" route, because I want to store some application state in each one.

After giving it some thought, it seems like the issues start here, at controllerAt:

https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/lib/controllers/array_controller.js#L183

If I do understand it well, it will create a different instance of itemController for each item and cache it in the subContainers array.

The visibility of this subContainers array is scoped to that particular ArrayController and so it's not possible to get one of those subContainers from a route.

Shouldn't this subContainer map be stored in the particular itemController class instead so it can be reused from different parts of the application?

Contributor

josepjaume commented Feb 11, 2013

My case scenario is the following:

I want to reuse the same instance of the controller on the list or in its "show" route, because I want to store some application state in each one.

After giving it some thought, it seems like the issues start here, at controllerAt:

https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/lib/controllers/array_controller.js#L183

If I do understand it well, it will create a different instance of itemController for each item and cache it in the subContainers array.

The visibility of this subContainers array is scoped to that particular ArrayController and so it's not possible to get one of those subContainers from a route.

Shouldn't this subContainer map be stored in the particular itemController class instead so it can be reused from different parts of the application?

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Feb 12, 2013

Member

So this is definitely a problem with the subContainer lookups. Unfortunately, I don't yet understand enough of this area to give a more conclusive answer. Hopefully @wycats or @tomdale can chime in here.

Member

wagenet commented Feb 12, 2013

So this is definitely a problem with the subContainer lookups. Unfortunately, I don't yet understand enough of this area to give a more conclusive answer. Hopefully @wycats or @tomdale can chime in here.

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Feb 12, 2013

Member

So here is a possible work around http://jsfiddle.net/sSdHm/66/

I definitely agree the behavior is surprising, especially the fact that it works on the first list render.
Thing is, this is how container inheritage works. If there is no controller instance in the parent container, it will create a new one (this is what is happening the first time). When you visit the show route, an instance is created in the parent (main) container. When you go back to the list view, you reset the collection and so you reset children containers. A new set of sub containers is created. But thing controller is already instantiated on the parent. So all of the items get the same instance of parent instantiated controller.

This is the explanation, what is the right thing to do, I am not sure. I get the case of reuse of controller in the show route. Seems like a valid idea. Seems like the solution would be to pass in to the route the caller controller container. It could work, but I am not sure about all the semantics.

Member

tchak commented Feb 12, 2013

So here is a possible work around http://jsfiddle.net/sSdHm/66/

I definitely agree the behavior is surprising, especially the fact that it works on the first list render.
Thing is, this is how container inheritage works. If there is no controller instance in the parent container, it will create a new one (this is what is happening the first time). When you visit the show route, an instance is created in the parent (main) container. When you go back to the list view, you reset the collection and so you reset children containers. A new set of sub containers is created. But thing controller is already instantiated on the parent. So all of the items get the same instance of parent instantiated controller.

This is the explanation, what is the right thing to do, I am not sure. I get the case of reuse of controller in the show route. Seems like a valid idea. Seems like the solution would be to pass in to the route the caller controller container. It could work, but I am not sure about all the semantics.

@josepjaume

This comment has been minimized.

Show comment
Hide comment
@josepjaume

josepjaume Feb 12, 2013

Contributor

Thanks for shining some light on it, @tchak. I actually ended up doing something like your fix, but inheriting a whole controller instead. Your workaround looks nicer though.

Yet as you point out this doesn't solve the case when you want to store something in the controller.

I think this is kind of an important issue to solve, specially since the Ember team seems to be encouraging using controllers to store application state:

#1637 (comment)

It would be awesome to do something like storing the controller's variables in a Client-Side HTML5 storage in a seamless way, for example.

Contributor

josepjaume commented Feb 12, 2013

Thanks for shining some light on it, @tchak. I actually ended up doing something like your fix, but inheriting a whole controller instead. Your workaround looks nicer though.

Yet as you point out this doesn't solve the case when you want to store something in the controller.

I think this is kind of an important issue to solve, specially since the Ember team seems to be encouraging using controllers to store application state:

#1637 (comment)

It would be awesome to do something like storing the controller's variables in a Client-Side HTML5 storage in a seamless way, for example.

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Feb 12, 2013

Member

@josepjaume the only clean way of doing it I see is using current controller container in the route.
Before going in to implementation details @tomdale or @wycats can you confirm/infirm on my idea?

Member

tchak commented Feb 12, 2013

@josepjaume the only clean way of doing it I see is using current controller container in the route.
Before going in to implementation details @tomdale or @wycats can you confirm/infirm on my idea?

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Feb 12, 2013

Member

@josepjaume I think my PR solves your use case

Member

tchak commented Feb 12, 2013

@josepjaume I think my PR solves your use case

@josepjaume

This comment has been minimized.

Show comment
Hide comment
@josepjaume

josepjaume Feb 12, 2013

Contributor

@tchak sorry but it doesn't seem like it :(

Contributor

josepjaume commented Feb 12, 2013

@tchak sorry but it doesn't seem like it :(

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Feb 12, 2013

Member

@josepjaume you have to change {{#linkTo thing item.content}} to {{#linkTo thing item}}

Member

tchak commented Feb 12, 2013

@josepjaume you have to change {{#linkTo thing item.content}} to {{#linkTo thing item}}

@josepjaume

This comment has been minimized.

Show comment
Hide comment
@josepjaume

josepjaume Feb 12, 2013

Contributor

Hmmm... Still doesn't work: http://jsfiddle.net/josepjaume/MxQnq/1/

Sorry!

Contributor

josepjaume commented Feb 12, 2013

Hmmm... Still doesn't work: http://jsfiddle.net/josepjaume/MxQnq/1/

Sorry!

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Feb 12, 2013

Member

@josepjaume I am sorry I forgot about the render() method ! Should be fixed !

Member

tchak commented Feb 12, 2013

@josepjaume I am sorry I forgot about the render() method ! Should be fixed !

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Feb 12, 2013

Member

There is one doubt left. The model hook will return a model (obviously) and not a controller, so in case you enter via url, the object controller will be set on the main container and it will fuck everything up... Not sure what to do about it. I will try to sleep on it.

Member

tchak commented Feb 12, 2013

There is one doubt left. The model hook will return a model (obviously) and not a controller, so in case you enter via url, the object controller will be set on the main container and it will fuck everything up... Not sure what to do about it. I will try to sleep on it.

tchak added a commit to tchak/ember.js that referenced this issue Feb 13, 2013

@ghost ghost assigned wycats Feb 14, 2013

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Feb 15, 2013

Member

For now, I think the workaround is to not use the same name. In this case, if you want to share, make a subclass of App.ThingController so that the naming is different. We definitely do need to fix this however.

Member

wagenet commented Feb 15, 2013

For now, I think the workaround is to not use the same name. In this case, if you want to share, make a subclass of App.ThingController so that the naming is different. We definitely do need to fix this however.

@caligo-mentis

This comment has been minimized.

Show comment
Hide comment
@caligo-mentis

caligo-mentis Feb 15, 2013

Contributor

@wagenet this is exactly what I did. Maybe we should use separate container for item controllers?

Contributor

caligo-mentis commented Feb 15, 2013

@wagenet this is exactly what I did. Maybe we should use separate container for item controllers?

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Feb 16, 2013

Member

@caligo-mentis I believe we do use a separate container but there's an inheritance issue for lookups. I'm not entirely sure how the internals work here yet.

Member

wagenet commented Feb 16, 2013

@caligo-mentis I believe we do use a separate container but there's an inheritance issue for lookups. I'm not entirely sure how the internals work here yet.

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Oct 12, 2013

Member

Can anyone confirm if this is still an issue on master? @wycats, any thoughts?

Member

wagenet commented Oct 12, 2013

Can anyone confirm if this is still an issue on master? @wycats, any thoughts?

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Nov 9, 2013

Member

Closing due to inactivity. If this is still an issue, please open a new ticket.

Member

wagenet commented Nov 9, 2013

Closing due to inactivity. If this is still an issue, please open a new ticket.

@wagenet wagenet closed this Nov 9, 2013

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Nov 9, 2013

Member

@wagenet we don't use a separate container. Sub Container life cycle pining proved a serious blocker to do entirely transparently. We will likely re-visit.

Also, unless im doing something wrong, the jsfiddle works fine on the latest release, so the problem seems to be resolved.

Member

stefanpenner commented Nov 9, 2013

@wagenet we don't use a separate container. Sub Container life cycle pining proved a serious blocker to do entirely transparently. We will likely re-visit.

Also, unless im doing something wrong, the jsfiddle works fine on the latest release, so the problem seems to be resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment