Lazily materialize DS.Models for app code, use InternalModel inside ED otherwise #3094
Conversation
| @@ -143,7 +148,7 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, { | |||
| this.get('relationship').removeRecords(records); | |||
| } | |||
| if (objects) { | |||
| this.get('relationship').addRecords(objects, idx); | |||
| this.get('relationship').addRecords(map(objects, function(obj) { return obj.reference; }), idx); | |||
|
Thanks @pangratz |
| @@ -887,8 +599,8 @@ var Model = Ember.Object.extend(Ember.Evented, { | |||
| and value is an [oldProp, newProp] array. | |||
| */ | |||
| changedAttributes: function() { | |||
| var oldData = get(this, '_data'); | |||
| var newData = get(this, '_attributes'); | |||
| var oldData = get(this.reference, '_data'); | |||
There was a problem hiding this comment.
it seems like this logic should move into the reference object rather than reaching into its private api.
something like Reference.prototype.changedAttributes. Then this function becomes this.reference.changedAttributes()
igorT
force-pushed
the
references
branch
4 times, most recently
from
c18d9fa
to
57074a4
Compare
igorT
changed the title
| }, | ||
|
|
||
| flushCanonical: function() { | ||
| //TODO make this smarter, currently its plenty stupid | ||
| var toSet = filter.call(this.canonicalState, function(record) { | ||
| return !record.get('isDeleted'); | ||
| return !record.isDeleted(); |
There was a problem hiding this comment.
Lets use internalModel here instead of record because record is already used both internally and externally to refer to an instance of a DS.Model.
|
|
||
| constructor: InternalModel, | ||
| materializeRecord: function() { | ||
| // lookupFactory should really return an object that creates |
There was a problem hiding this comment.
should we guard + throw if a re-materialization occurs? That seems like an error we would like to fail fast on.
|
@igorT there is lot's of places where |
| }, | ||
|
|
||
| becameReady: function() { | ||
| var self = this; |
There was a problem hiding this comment.
Self is a real thing in the bowser / webWorkers. we should likely not shadow it. Instead lets call this internalModel
Or even better, use the non-closure retaining form of schedule
Ember.run.schedule('actions', this, this.store.recordArrayManager.recordWasLoaded, this)There was a problem hiding this comment.
Ember.run.schedule('actions', this.store.recordArrayManager, this.store.recordArrayManager.recordWasLoaded, this) was the right call
|
I provided some superficial feedback. It is a bit tricky to provide something more in-depth for such a large set of changes. I will try to do another pass in the morning. |
|
@stefanpenner the annoying part is that the large part is just moving methods from model to internalModel but not sure how to make it easier to review. You did review some of old code and now we can improve it ;) |
… DS.Model This commit adds an `InternalModel` class that is now used everywhere inside ED to represent models. This class is a fast Javascript object that contains all the data that we know for a particular record. At the ED/App code boundaries, such as responses to `find`, models being set/pushed to `belongsTo`/`hasMany` we convert between internalModels and DS.Models. This should be a huge performance win, because we now lazily create DS.Models which are pretty slow to instantiate. We need to wait for the new serializer/push refactor in order to use a `push` that does not immediately materialize a record to get further perf gains. For now most of perf gains if for foreign keys.
Lazily materialize DS.Models for app code, use InternalModel inside ED otherwise
No description provided.