[BUGFIX] Ensure that dynamic injections are not cacheable in a Container. #10173
Conversation
|
LGTM, @stefanpenner - r? |
| @@ -182,6 +182,10 @@ Container.prototype = { | |||
| } | |||
| })(); | |||
|
|
|||
| function isSingleton(container, fullName) { | |||
| return container._registry.getOption(fullName, 'singleton') !== false; | |||
There was a problem hiding this comment.
return !container._registry.getOption(fullName, 'singleton') perhaps?
There was a problem hiding this comment.
or even return container._registry.getOption(fullName, 'singleton') if I'm reading this code correctly.
There was a problem hiding this comment.
The singleton option by default is true, so neither alternative above would work for all cases (true, false, undefined). One alternative that would work is return !!container._registry.getOption(fullName, 'singleton'); - I'm guessing that's what you meant with your first suggestion. Anyway, I'd be glad to change to this if it's more a more common convention throughout ember. There are other instances of checking !== false throughout the Container which I could also change to !! if there's consensus on this.
There was a problem hiding this comment.
On second thought, I don't see an upside to being more accepting of truthy / falsy values in settings completely controlled by developers (which should therefore have explicit values like false), so I'd recommend keeping it as is.
There was a problem hiding this comment.
Agreed. Let's keep it matching the exact value specified.
|
@dgeb - This looks good, but needs a rebase. Can you confirm this is still needed (after the container/registry reform PR's) and rebase ? |
|
@dgeb - Ping? |
A non-singleton injection should prevent caching of an object or factory instance in a container. [Fixes emberjs#3310]
|
@rwjblue I can confirm this is still needed via the test that's included, which fails if injections aren't marked as dynamic. I've just rebased against master. |
[BUGFIX] Ensure that dynamic injections are not cacheable in a Container.
|
Thank you @dgeb! |
A non-singleton injection should prevent caching of an object or factory instance in a container.
Fixes #3310