Make all model lookups go through the container #2907

fivetanley · 2015-03-20T04:27:18Z

move factories off constructor and delegate to the container for lookups

This commit changes the way factories are located by Ember Data.
Previously, factories were stuffed onto the constructor of a
DS.Model subclass.

This was problematic for a few reasons:

Classes for relationships defined via hasMany(Object) (instead of
hasMany('string-name') were different than what was in the container.
This caused several issues for Ember CLI and Globals Mode users alike.
Inconsistency with the rest of the way lookups work in Ember.
Zalgo

Now, relationship information can be accessed off the "type" in the
relationships "meta" hash. If you need to lookup a factory in your app,
you need to use this.store.modelFor, or this.store.modelFactoryFor if
you prefer to check that the model exists without throwing an exception.
This change makes all model factory lookups go through the container.
This should hopefully kill Ember.MODEL_FACTORY_INJECTIONS = true;.

This commit changes the way factories are located by Ember Data. Previously, factories were stuffed onto the constructor of a DS.Model subclass. This was problematic for a few reasons: - Classes for relationships defined via hasMany(Object) (instead of hasMany('string-name') were different than what was in the container. This caused several issues for Ember CLI and Globals Mode users alike. - Inconsistency with the rest of the way lookups work in Ember. - Zalgo Now, relationship information can be accessed off the "type" in the relationships "meta" hash. If you need to lookup a factory in your app, you need to use this.store.modelFor, or this.store.modelFactoryFor if you prefer to check that the model exists without throwing an exception. This change makes all model factory lookups go through the container. This should hopefully kill `Ember.MODEL_FACTORY_INJECTIONS = true;`.

…lt-export

fivetanley · 2015-03-20T04:28:59Z

cc @igorT @mixonic @stefanpenner

fivetanley · 2015-03-20T04:30:52Z

packages/ember-data/lib/serializers/json-serializer.js

@@ -158,13 +158,14 @@ export default Serializer.extend({
  */
  normalize: function(type, hash) {
    if (!hash) { return hash; }
+    var model = this.store.modelFor(type);


not sure at this point if serializers should receive type or typeKey.

While we are at it, can we maybe figure out a better name than model for the actual class. Something like using type & typeKey or typeClass. Model implies an actual record, very easy to get confused

stefanpenner · 2015-03-20T06:40:35Z

This looks great.

so to confirm, no more instances (like store) now exist on the factories?

fivetanley · 2015-03-20T15:17:48Z

@bmac I'm removing this from the milestone. It needs some battletesting before we bring it in.

stefanpenner · 2015-03-28T00:23:47Z

packages/ember-data/lib/adapters/rest-adapter.js

@@ -502,7 +502,7 @@ export default Adapter.extend(BuildURLMixin, {
  */
  findBelongsTo: function(store, record, url, relationship) {
    var id   = get(record, 'id');
-    var type = record.constructor.typeKey;
+    var type = store.modelFor(record.constructor.typeKey);


record.constructor is the type

…lt-export

kball · 2015-04-04T16:55:29Z

packages/ember-data/lib/system/store.js

@@ -1445,8 +1447,9 @@ Store = Service.extend({
  },

  modelFactoryFor: function(key) {
-    if (this.container.has('model:' + key)) {
-      return this.container.lookupFactory('model:' + key);
+    var singularized = singularize(key);


In some places we're using singularized, and other places fully normalizing with this._normalizeTypeKey. Shouldn't we normalize the same way throughout?

That is a good point! looking into it now. Thanks for taking the time to review! Makes me more confident that we'll catch more things before this gets merged in.

kball · 2015-04-04T17:08:52Z

packages/ember-data/lib/system/relationships/ext.js

-      var relationship = relationshipFromMeta(this.store, meta);
-      relationship.type = typeForRelationshipMeta(this.store, meta);
+      var relationship = relationshipFromMeta(meta);
+      relationship.type = typeForRelationshipMeta(meta);


I believe this is extraneous. Looking at relationshipFromMeta it appears to already define type using typeForRelationshipMeta

fivetanley · 2015-04-05T22:11:17Z

ping @igorT @stefanpenner

…lt-export

fivetanley · 2015-04-10T19:27:33Z

you can try this out by using the following in your bower.json file:

{
  "ember-data": "components/ember-data#always-lookup-models-through-container"
}

If you can try this out in your app, I can pair with you to see if we can catch some bugs early.

fivetanley · 2015-04-15T00:05:23Z

@stefanpenner i see you are reading your email :P

stefanpenner · 2015-04-15T00:06:43Z

i'll do one pass now, then another with manual testing in-app later :)

stefanpenner · 2015-04-15T00:18:33Z

got distracted by es7 decorators, will look later :P

igorT · 2015-04-17T19:30:07Z

packages/ember-data/lib/serializers/rest-serializer.js

@@ -282,7 +282,7 @@ var RESTSerializer = JSONSerializer.extend({

      // legacy support for singular resources
      if (isPrimary && Ember.typeOf(value) !== "array" ) {
-        primaryRecord = this.normalize(primaryType, value, prop);
+        primaryRecord = this.normalize(type, value, prop);


Why this change? Shouldn't this be primary type?

i think so, yeah. i'll change it back.

Stanley Stuart added 2 commits March 19, 2015 23:22

Merge branch 'master' of github.com:emberjs/data into fix-model-defau…

5692052

…lt-export

fivetanley reviewed Mar 20, 2015
View reviewed changes

bmac added this to the 1.0.0-beta.16 milestone Mar 20, 2015

fivetanley removed this from the 1.0.0-beta.16 milestone Mar 20, 2015

igorT mentioned this pull request Mar 27, 2015

[Meta issue] Ship 1.0 #2545

Closed

28 tasks

stefanpenner reviewed Mar 28, 2015
View reviewed changes

Stanley Stuart added 2 commits April 4, 2015 11:45

rename instances of modelClass to be typeClass for consistency

2d88fa5

Merge branch 'master' of github.com:emberjs/data into fix-model-defau…

05701f4

…lt-export

kball reviewed Apr 4, 2015
View reviewed changes

use modelFor when looking up types in ManyArray#createRecord

42befb8

kball reviewed Apr 4, 2015
View reviewed changes

consolidate typeKey normalization to its own file

48cc8fc

Stanley Stuart added 3 commits April 10, 2015 11:24

Merge branch 'master' of github.com:emberjs/data into fix-model-defau…

948022e

…lt-export

don't change the public API of serializers/adapters, instead pass types

19e5147

rollback bad doc changes for serializers

a75d90a

igorT reviewed Apr 17, 2015
View reviewed changes

fivetanley closed this May 18, 2015

stefanpenner deleted the fix-model-default-export branch April 18, 2017 03:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make all model lookups go through the container #2907

Make all model lookups go through the container #2907

fivetanley commented Mar 20, 2015

fivetanley commented Mar 20, 2015

fivetanley Mar 20, 2015

igorT Mar 23, 2015

stefanpenner commented Mar 20, 2015

fivetanley commented Mar 20, 2015

stefanpenner Mar 28, 2015

kball Apr 4, 2015

fivetanley Apr 4, 2015

kball Apr 4, 2015

fivetanley commented Apr 5, 2015

fivetanley commented Apr 10, 2015

fivetanley commented Apr 15, 2015

stefanpenner commented Apr 15, 2015

stefanpenner commented Apr 15, 2015

igorT Apr 17, 2015

fivetanley Apr 17, 2015

Make all model lookups go through the container #2907

Make all model lookups go through the container #2907

Conversation

fivetanley commented Mar 20, 2015

fivetanley commented Mar 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner commented Mar 20, 2015

fivetanley commented Mar 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetanley commented Apr 5, 2015

fivetanley commented Apr 10, 2015

fivetanley commented Apr 15, 2015

stefanpenner commented Apr 15, 2015

stefanpenner commented Apr 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment