Skip to content
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

Dependency Injections are broken when using ember 2.12 and newer. #4807

Closed
workmanw opened this issue Feb 20, 2017 · 6 comments
Closed

Dependency Injections are broken when using ember 2.12 and newer. #4807

workmanw opened this issue Feb 20, 2017 · 6 comments
Assignees

Comments

@workmanw
Copy link

Originally filed as: emberjs/ember.js#14941
Ember-Release (2.11.x): Working Twiddle
Ember-Beta (2.12.x): Broken Twiddle

It seems that using owner.inject targeting a model type is broken w/ ember-beta. Example:

// app/initializers/i18n.js
export default {
  name: 'i18n',
  after: 'ember-i18n',
  initialize: function(app) {
    app.inject('model', 'i18n', 'service:i18n')
  }
};

Additionally, other ember types, such as component and controller, work as expected.


@rwjblue helped me on slack to track this down a bit. We believe the cause of the problem lies with the implementation of modelForFactory (store#modelForFactory).

Prior to Ember 2.11, ember-data used _lookupFactory and internally that would do all of the dependency injections via an expensive .extend() process. One benefit of factoryFor is that it does not do this and thus saves perf. But the consequence is that when instantiating a class extracted from a "factory manager" those injections still need to be setup.

@rwjblue
Copy link
Member

rwjblue commented Feb 20, 2017

Here are a few of the steps on the code safari that @workmanw and I just went through:

  • store.modelForFactory returns the raw .class property instead of the full result from owner.factoryFor
  • InternalModel#getRecord calls ._create on the result of store.modelFactoryFor to instantiate the model instance. Unfortunately, this ._create is completely unaware of injections and is simply calling ._create on the raw unmodified .class.

Most likely we should refactor to return the actual owner.factoryFor('model:foo') result (instead of owner.factoryFor('model:foo').class), however this will pose some issues with the following:

  • store._modelForMixin mutates the return value of store.modelFactoryFor to add __mixin and __isMixin properties to the factory.
  • store. _modelFor mutates the return value of store.modelFactoryFor (to add the modelName that was used to look up the factory

@rwjblue
Copy link
Member

rwjblue commented Feb 20, 2017

store._modelForMixin mutates the return value of store.modelFactoryFor to add __mixin and __isMixin properties to the factory.

I submitted #4808 to remove this as an issue.

@stefanpenner
Copy link
Member

I have some cycles, working on this now.

@stefanpenner stefanpenner self-assigned this Feb 21, 2017
@workmanw
Copy link
Author

@stefanpenner Thanks a lot for jumping on this. I'm happy to help in any way I can.

stefanpenner added a commit that referenced this issue Feb 21, 2017
*note: this combined with embers factoryFor makes `MODEL_FACTORY_INJECTIONS` irrelevant.*
stefanpenner added a commit that referenced this issue Feb 21, 2017
*note: this combined with embers factoryFor makes `MODEL_FACTORY_INJECTIONS` irrelevant.*
stefanpenner added a commit that referenced this issue Feb 21, 2017
*note: this combined with embers factoryFor makes `MODEL_FACTORY_INJECTIONS` irrelevant.*
stefanpenner added a commit that referenced this issue Feb 21, 2017
*note: this combined with embers factoryFor makes `MODEL_FACTORY_INJECTIONS` irrelevant.*
@stefanpenner
Copy link
Member

@workmanw if you have cycles, test out my PR #4810 and let me know if I missed anything.

stefanpenner added a commit that referenced this issue Feb 22, 2017
*note: this combined with embers factoryFor makes `MODEL_FACTORY_INJECTIONS` irrelevant.*
bmac added a commit that referenced this issue Feb 22, 2017
[Fixes #4807] realize class + factory seperation
stefanpenner added a commit that referenced this issue Feb 23, 2017
*note: this combined with embers factoryFor makes `MODEL_FACTORY_INJECTIONS` irrelevant.*
stefanpenner added a commit that referenced this issue Feb 23, 2017
*note: this combined with embers factoryFor makes `MODEL_FACTORY_INJECTIONS` irrelevant.*
stefanpenner added a commit that referenced this issue Feb 23, 2017
*note: this combined with embers factoryFor makes `MODEL_FACTORY_INJECTIONS` irrelevant.*
@rwjblue
Copy link
Member

rwjblue commented Feb 24, 2017

FYI - #4820 backports the fix to release branch (which will likely be released as a 2.11.x bugfix release).

bmac pushed a commit that referenced this issue Feb 24, 2017
*note: this combined with embers factoryFor makes `MODEL_FACTORY_INJECTIONS` irrelevant.*
(cherry picked from commit 314ae0b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants