Non singleton injections are treated as singleton

[taras]

I'm trying to figure out if I found a bug or I'm just misunderstanding register/inject functionality.

JSBin shows multiple instances of person are created using lookup and I would expect for each person to have a unique instance of App.FavoriteFruit because its injected into person as a non-singleton injection.

The failing test shows that different instances of people are created but they share an instance of favorite fruit.

Is this a bug? ( tests don't cover this scenario )

[stefanpenner]

by default all lookups return a singleton per container.

public api: https://github.com/emberjs/ember.js/blob/master/packages/ember-application/lib/system/application.js#L373-L399
internal container api: https://github.com/emberjs/ember.js/blob/master/packages/container/lib/main.js#L243-L253

[taras]

Ok

[stefanpenner]

@taras I'm available on irc #emberjs if you want further clarification. This part of ember does lack guides.

[taras]

@stefanpenner I tried to find you on IRC but I don't know your handle.

I'm not sure if you looked at my JSBin, but it seems like a very awkward inconsistency to support singleton:false with container.lookup() but not when it's injected.

Here is original http://jsbin.com/aguLeDi/11/edit and another http://jsbin.com/usaluc/12/edit by @intuitivepixel that shows singleton injections working when injected into controllers.

Could you please take another look at this?

[stefanpenner]

reopening, till i have a chance to digest the above request.

[lukemelia]

@stefanpenner @taras I've narrowed down a failing test for this: https://github.com/lukemelia/ember.js/compare/injections-fix?expand=1

The issue is in the factory caching in factoryFor (https://github.com/emberjs/ember.js/blob/master/packages/container/lib/main.js#L802). It doesn't take into account the fact that the injections may be singleton: false. Commenting out that lines makes the test pass, but obviously a more nuanced fix would be preferable. Thoughts?

[stefanpenner]

@lukemelia thanks for digging into this

Ya, commenting that out would mean every instance gets it own factory, which is non-deal.

I have a proper solution in mind. Specifically, the work @kselden and I have been discussing would solve this.

[taras]

@lukemelia thank you for looking into this.

[wagenet]

@stefanpenner is this fixed?

[wagenet]

@stefanpenner ping.

[stefanpenner]

not fixed, will be part of the next container related refactor.

[wagenet]

@stefanpenner fixed yet?

[stefanpenner]

no

[wagenet]

#5266