-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Remove internal factoryInjection API #14816
Conversation
locks
commented
Jan 11, 2017
•
edited
Loading
edited
- Container: A factory returned from lookupFactory has a debugkey
- Ember.Application Dependency Injection – toString: instances
- Ember.Application Dependency Injection – toString: with a custom resolver
9e40ee1
to
7041640
Compare
Remove factoryInjections related code Kill lookupFactory debugkey test Add NAME_KEY to injectedFactory class
275739b
to
d69f8d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with all of this work, but I'd really like to see us remove that last reopenClass
in deprecatedFactoryFor
. Though maybe it isn't worth a ton of effort since we plan to remove that whole chunk of code with the ember-factory-for
feature flag...
packages/container/lib/container.js
Outdated
|
||
let injectedFactory = factory.extend(injections); | ||
|
||
// TODO - remove all `container` injections when Ember reaches v3.0.0 | ||
injectDeprecatedContainer(injectedFactory.prototype, container); | ||
injectedFactory.reopenClass(factoryInjections); | ||
injectedFactory.reopenClass({ [NAME_KEY]: registry.makeToString(factory, fullName) }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we can get rid of this also (I'm not sure we have solved the "problem" of factory injections if we cannot remove this reopenClass
).
☔ The latest upstream changes (presumably #15014) made this pull request unmergeable. Please resolve the merge conflicts. |
@locks ping |
@stefanpenner there was a problem with 5b2dc4d that I don't know how to address. I think it particularly broke some of Ember Data's output. |
☔ The latest upstream changes (presumably #15177) made this pull request unmergeable. Please resolve the merge conflicts. |
@locks can you enumerate the problem, steps to reproduce? |
I believe this will be replaced by #15193. |