emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.46k stars 4.22k forks source link

Remove MODEL_FACTORY_INJECTIONS #9500

Closed wycats closed 6 years ago

wycats commented 9 years ago

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

stefanpenner commented 9 years ago

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

fivetanley commented 9 years ago

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 commented 9 years ago

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 EXTENDPROTOTYPES = true, as we get a new subclass each test run. But unfortunately `EXTEND...` breaks the current implementation of polymorphism

wycats commented 9 years ago

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

tomdale commented 9 years ago

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 commented 9 years ago

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

stefanpenner commented 9 years ago

constructor has some meaning in javascript, what about Class.

factoryObject = {
  Class: OriginalClassObject,
  create(attrs) { ... create instance with injections }
};
tomdale commented 9 years ago

@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 commented 9 years ago

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

igorT commented 9 years ago

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 commented 9 years ago

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

rwjblue commented 8 years ago

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 commented 8 years ago

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 commented 7 years ago

https://github.com/emberjs/data/pull/4810 + https://github.com/emberjs/ember.js/pull/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 commented 7 years ago

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

mmun commented 6 years ago

MODEL_FACTORY_INJECTIONS was removed in 2.14.