[BUGFIX release] Don’t leak container while injection deprecated cont… #15142
Conversation
|
The approach looks good, but there is some sort of failure still... |
packages/container/lib/container.js
Outdated
| // TODO - remove when Ember reaches v3.0.0 | ||
| function injectDeprecatedContainer(object) { | ||
| if ('container' in object) { return; } | ||
| Object.defineProperty(object, 'container', INJECTED_DEPRECATED_CONTAINER_DESC); |
There was a problem hiding this comment.
also I need to actually set the container again here setOwner(object, container)
There was a problem hiding this comment.
looks like container.owner is null for the offending tests. and this PR closing over container masked that.
@rwjblue do i expect dont worry, i was just short on time I'll take a look once im home.container.owner to be null? Is that a test config error?
There was a problem hiding this comment.
I'll actually investigate more, just gotta head out now.
stefanpenner
force-pushed
the
dont-leak-container
branch
2 times, most recently
from
1509453 to
e9982b3
Compare
|
@rwjblue / @krisselden fixed, ready for review |
stefanpenner
force-pushed
the
dont-leak-container
branch
from
e9982b3 to
e951128
Compare
|
Because the original code was explicitly returning the container (Not whatever the owner happened to be) I didn't want to change behavior for a leak fix. I think as a next step we should likely just remove this code entirely in canary, thoughts? |
|
Or a better question, what would need to be done to remove this in canary? It may just be update the deprecation message in release/beta |
…ainer