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

Remove MODEL_FACTORY_INJECTIONS #9500

Closed
wycats opened this issue Nov 5, 2014 · 16 comments
Closed

Remove MODEL_FACTORY_INJECTIONS #9500

wycats opened this issue Nov 5, 2014 · 16 comments
Labels

Comments

@wycats
Copy link
Member

wycats commented Nov 5, 2014

What are the blockers for normalizing model instantiation to be like all other injections?

@stefanpenner
Copy link
Member

  • ember-data stores stuff on the constructors
  • our current subclassing mechanism for injectable factories breaks ember-data's current polymorphic setup (and potentially reasonable user-code)
  • maybe transition to a specialized factory object (but this is theoretically breaking, and requires much changing to the view layer. Specifically this.createChildView mega morphic signature)
  • ...

i'll list more and a migration plan as I have time.

@fivetanley
Copy link
Member

ember-data stores stuff on the constructors

I feel like this could be actionable immediately (I'm saying I could try to do it) if we put stuff on the (Ember, not Ember Data) meta for the class. Is that a valid assumption?

@stefanpenner
Copy link
Member

I think its tricky since it cache various relationship things as CP's on the constructor. This caching is important re: perf, but also pollutes the constructor. This is fixed by todays EXTEND_PROTOTYPES = true, as we get a new subclass each test run. But unfortunately EXTEND_... breaks the current implementation of polymorphism

@wycats
Copy link
Member Author

wycats commented Nov 5, 2014

@fivetanley Can we make sure we discuss an alternative for storing metadata on classes at the next ED meeting?

@tomdale
Copy link
Member

tomdale commented Nov 5, 2014

Perhaps the factory returned from the container, in addition to having a create() method, could have a constructor property to access the actual class being used to make instances? This is useful for informational/metadata purposes but we can make it clear you shouldn't use it to create instances. (Not sure anyone would be tempted to container.lookupFactory('model:foo').constructor.create() and expect the correct behavior, but…)

@fivetanley
Copy link
Member

I've made a note of it and will bring it up next week.

@stefanpenner
Copy link
Member

constructor has some meaning in javascript, what about Class.

factoryObject = {
  Class: OriginalClassObject,
  create(attrs) { ... create instance with injections }
};

@tomdale
Copy link
Member

tomdale commented Nov 12, 2014

@stefanpenner Great point. Uppercase "C" looks a little weird to me—what about lowercase "c" class (should be fine since we use ES3safe) or factory?

@stefanpenner
Copy link
Member

@tomdale ya it should be safe with ES3Safe and modern browsers

@igorT
Copy link
Member

igorT commented Nov 13, 2014

Currently in ED in a lot of places where we use modelFor(mostly in Serializer/Adapter), we actually want the class not the factory, as we don't instantiate stuff ourselves, just lookup metadata

@stefanpenner
Copy link
Member

@igorT @bmac @fivetanley whats the status for ember-data on this one?

@rwjblue
Copy link
Member

rwjblue commented Apr 18, 2016

I believe a bunch of work in ED has been done on this. I'd like to get some feedback from @emberjs/ember-data-contributors...

@stefanpenner
Copy link
Member

The remaining work was removing the fact the subclasses are returned from lookup factory. Unfortunately others decided this was for some reason to much of a breaking change during 2.0 transition

@stefanpenner
Copy link
Member

stefanpenner commented Feb 23, 2017

emberjs/data#4810 + #14360 together make Ember.MODEL_FACTORY_INJECTIONS have no more power (it not longer does stuff). We likely can't remove it for some time from the blueprints for compatibility with individuals on older versions of ember or ember-data.

cc @chadhietala

@rwjblue
Copy link
Member

rwjblue commented Feb 23, 2017

We can deprecate having it set though. This sets us up for removal at some future point (sometime after 2.16).

@mmun
Copy link
Member

mmun commented Feb 24, 2018

MODEL_FACTORY_INJECTIONS was removed in 2.14.

@mmun mmun closed this as completed Feb 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants