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

[Feat] simplified relationship state #4882

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 6 additions & 70 deletions addon/-private/system/many-array.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@
import Ember from 'ember';
import { assert } from 'ember-data/-debug';
import { PromiseArray } from "./promise-proxies";
import { _objectIsAlive } from "./store/common";
import diffArray from './diff-array';

const { get } = Ember;

Expand Down Expand Up @@ -62,7 +60,7 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, {
@property {Boolean} isLoaded
*/
this.isLoaded = false;
this.length = 0;
this.length = this.currentState.length;

/**
Used for async `hasMany` arrays
Expand Down Expand Up @@ -128,9 +126,6 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, {
@private
*/
this.relationship = this.relationship || null;

this.currentState = [];
this.flushCanonical(false);
},

objectAt(index) {
Expand All @@ -140,74 +135,15 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, {
return internalModel.getRecord();
},

flushCanonical(isInitialized = true) {
// It’s possible the parent side of the relationship may have been unloaded by this point
if (!_objectIsAlive(this)) {
return;
}
let toSet = this.canonicalState;

//a hack for not removing new records
//TODO remove once we have proper diffing
let newInternalModels = this.currentState.filter(
// only add new internalModels which are not yet in the canonical state of this
// relationship (a new internalModel can be in the canonical state if it has
// been 'acknowleged' to be in the relationship via a store.push)
(internalModel) => internalModel.isNew() && toSet.indexOf(internalModel) === -1
);
toSet = toSet.concat(newInternalModels);

// diff to find changes
let diff = diffArray(this.currentState, toSet);

if (diff.firstChangeIndex !== null) { // it's null if no change found
// we found a change
this.arrayContentWillChange(diff.firstChangeIndex, diff.removedCount, diff.addedCount);
this.set('length', toSet.length);
this.currentState = toSet;
this.arrayContentDidChange(diff.firstChangeIndex, diff.removedCount, diff.addedCount);
if (isInitialized && diff.addedCount > 0) {
//notify only on additions
//TODO only notify if unloaded
this.relationship.notifyHasManyChanged();
}
}
},

internalReplace(idx, amt, objects) {
if (!objects) {
objects = [];
}
this.arrayContentWillChange(idx, amt, objects.length);
this.currentState.splice.apply(this.currentState, [idx, amt].concat(objects));
this.set('length', this.currentState.length);
this.arrayContentDidChange(idx, amt, objects.length);
},

//TODO(Igor) optimize
_removeInternalModels(internalModels) {
for (let i=0; i < internalModels.length; i++) {
let index = this.currentState.indexOf(internalModels[i]);
this.internalReplace(index, 1);
}
},

//TODO(Igor) optimize
_addInternalModels(internalModels, idx) {
if (idx === undefined) {
idx = this.currentState.length;
}
this.internalReplace(idx, 0, internalModels);
},

replace(idx, amt, objects) {
let internalModels;
if (amt > 0) {
internalModels = this.currentState.slice(idx, idx+amt);
this.get('relationship').removeInternalModels(internalModels);
}
if (objects) {
this.get('relationship').addInternalModels(objects.map(obj => obj._internalModel), idx);
let internalModels = objects.map(obj => obj._internalModel);
this.relationship.addInternalModels(internalModels, idx);
}
},

Expand Down Expand Up @@ -260,8 +196,8 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, {
save() {
let manyArray = this;
let promiseLabel = 'DS: ManyArray#save ' + get(this, 'type');
let promise = Ember.RSVP.all(this.invoke("save"), promiseLabel).
then(() => manyArray, null, 'DS: ManyArray#save return ManyArray');
let promise = Ember.RSVP.all(this.invoke("save"), promiseLabel)
.then(() => manyArray, null, 'DS: ManyArray#save return ManyArray');

return PromiseArray.create({ promise });
},
Expand All @@ -278,7 +214,7 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, {
const store = get(this, 'store');
const type = get(this, 'type');

assert(`You cannot add '${type.modelName}' records to this polymorphic relationship.`, !get(this, 'isPolymorphic'));
assert(`You cannot add '${type.modelName}' records to this polymorphic relationship.`, !this.isPolymorphic);
let record = store.createRecord(type.modelName, hash);
this.pushObject(record);

Expand Down
25 changes: 19 additions & 6 deletions addon/-private/system/model/internal-model.js
Original file line number Diff line number Diff line change
Expand Up @@ -427,17 +427,30 @@ export default class InternalModel {
to or has many.
*/
_directlyRelatedInternalModels() {
let array = [];
this.type.eachRelationship((key, relationship) => {
let set = new OrderedSet();

this.modelClass.eachRelationship((key) => {
if (this._relationships.has(key)) {
let relationship = this._relationships.get(key);
let localRelationships = relationship.members.toArray();
let serverRelationships = relationship.canonicalMembers.toArray();

array = array.concat(localRelationships, serverRelationships);
switch (relationship.kind) {
case 'belongsTo':
set.pushMany([relationship.currentState, relationship.canonicalState]);
return;
case 'hasMany':
set.pushMany(relationship.currentState);
set.pushMany(relationship.canonicalState);
return;
case 'implicit':
default:
set.pushMany(relationship.currentState.list);
set.pushMany(relationship.canonicalState.list);
return;
}
}
});
return array;

return set.list;
}


Expand Down
12 changes: 12 additions & 0 deletions addon/-private/system/ordered-set.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,18 @@ OrderedSet.prototype = Object.create(EmberOrderedSet.prototype);
OrderedSet.prototype.constructor = OrderedSet;
OrderedSet.prototype._super$constructor = EmberOrderedSet;

OrderedSet.prototype.pushMany = function pushMany(additions) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unit test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

for (let i = 0; i < additions.length; i++) {
let value = additions[i];

if (value) {
this.add(value);
}
}

return this.size;
};

OrderedSet.prototype.addWithIndex = function(obj, idx) {
let guid = guidFor(obj);
let presenceSet = this.presenceSet;
Copy link
Member

@stefanpenner stefanpenner Apr 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unfortunately private API that will change. Ember-data shouldn't be reaching into private state.. hmmm

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only thing is to add this to ember's, and have ember's lazily create presenceSet for old versions of ED.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is pre-existing, but I agree. I feel we really just need to build an OrderedSet with good perf and tailored to ember-data, given that it exists inside of Ember mostly for ember-data to begin with.

Expand Down
6 changes: 3 additions & 3 deletions addon/-private/system/references/belongs-to.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ BelongsToReference.prototype.remoteType = function() {
@return {String} The id of the record in this belongsTo relationship.
*/
BelongsToReference.prototype.id = function() {
let inverseInternalModel = this.belongsToRelationship.inverseInternalModel;
let inverseInternalModel = this.belongsToRelationship.currentState;
return inverseInternalModel && inverseInternalModel.id;
};

Expand Down Expand Up @@ -312,7 +312,7 @@ BelongsToReference.prototype.push = function(objectOrPromise) {
@return {DS.Model} the record in this relationship
*/
BelongsToReference.prototype.value = function() {
let inverseInternalModel = this.belongsToRelationship.inverseInternalModel;
let inverseInternalModel = this.belongsToRelationship.currentState;

if (inverseInternalModel && inverseInternalModel.isLoaded()) {
return inverseInternalModel.getRecord();
Expand Down Expand Up @@ -403,7 +403,7 @@ BelongsToReference.prototype.load = function() {
@return {Promise} a promise that resolves with the record in this belongs-to relationship after the reload has completed.
*/
BelongsToReference.prototype.reload = function() {
return this.belongsToRelationship.reload().then((internalModel) => {
return this.belongsToRelationship.reload().then(() => {
return this.value();
});
};
Expand Down
10 changes: 5 additions & 5 deletions addon/-private/system/references/has-many.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,9 @@ HasManyReference.prototype.link = function() {
@return {Array} The ids in this has-many relationship
*/
HasManyReference.prototype.ids = function() {
let members = this.hasManyRelationship.members.toArray();
let currentState = this.hasManyRelationship.currentState;

return members.map(function(internalModel) {
return currentState.map(function(internalModel) {
return internalModel.id;
});
};
Expand Down Expand Up @@ -297,7 +297,7 @@ HasManyReference.prototype.push = function(objectOrPromise) {
});
}

this.hasManyRelationship.computeChanges(internalModels);
this.hasManyRelationship.updateInternalModelsFromAdapter(internalModels);

return this.hasManyRelationship.manyArray;
});
Expand All @@ -309,9 +309,9 @@ HasManyReference.prototype._isLoaded = function() {
return false;
}

let members = this.hasManyRelationship.members.toArray();
let currentState = this.hasManyRelationship.currentState;

return members.every(function(internalModel) {
return currentState.every(function(internalModel) {
return internalModel.isLoaded() === true;
});
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,17 @@ export default class RelationshipPayloadsManager {

let modelClass = this._store._modelFor(modelName);
let relationshipsByName = get(modelClass, 'relationshipsByName');
Object.keys(relationshipsData).forEach(key => {

let relKeys = Object.keys(relationshipsData);

for (let i = 0; i < relKeys.length; i++) {
let key = relKeys[i];

let relationshipPayloads = this._getRelationshipPayloads(modelName, key, modelClass, relationshipsByName, true);
if (relationshipPayloads) {
relationshipPayloads.push(modelName, id, key, relationshipsData[key]);
}
});
}
}

/**
Expand All @@ -133,12 +138,15 @@ export default class RelationshipPayloadsManager {
unload(modelName, id) {
let modelClass = this._store._modelFor(modelName);
let relationshipsByName = get(modelClass, 'relationshipsByName');
relationshipsByName.forEach((_, relationshipName) => {
let relationshipNames = relationshipsByName._keys.list;

for (let i = 0; i < relationshipNames.length; i++) {
let relationshipName = relationshipNames[i];
let relationshipPayloads = this._getRelationshipPayloads(modelName, relationshipName, modelClass, relationshipsByName, false);
if (relationshipPayloads) {
relationshipPayloads.unload(modelName, id, relationshipName);
}
});
}
}

/**
Expand Down Expand Up @@ -195,7 +203,7 @@ export default class RelationshipPayloadsManager {
// a) the inverse model name
//- b) the name of the inverse relationship
if (inverseMeta) {
inverseRelationshipName = inverseMeta.name
inverseRelationshipName = inverseMeta.name;
inverseModelName = relationshipMeta.type;
inverseRelationshipMeta = get(inverseMeta.type, 'relationshipsByName').get(inverseRelationshipName);
} else {
Expand Down
16 changes: 8 additions & 8 deletions addon/-private/system/relationships/relationship-payloads.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ export default class RelationshipPayloads {
@method
*/
push(modelName, id, relationshipName, relationshipData) {
this._pendingPayloads.push([modelName, id, relationshipName, relationshipData]);
this._pendingPayloads.push(modelName, id, relationshipName, relationshipData);
}

/**
Expand Down Expand Up @@ -150,19 +150,19 @@ export default class RelationshipPayloads {
if (this._pendingPayloads.length === 0) { return; }

let payloadsToBeProcessed = this._pendingPayloads.splice(0, this._pendingPayloads.length);
for (let i=0; i<payloadsToBeProcessed.length; ++i) {
let modelName = payloadsToBeProcessed[i][0];
let id = payloadsToBeProcessed[i][1];
let relationshipName = payloadsToBeProcessed[i][2];
let relationshipData = payloadsToBeProcessed[i][3];
for (let i = 0; i < payloadsToBeProcessed.length; i += 4) {
let modelName = payloadsToBeProcessed[i];
let id = payloadsToBeProcessed[i + 1];
let relationshipName = payloadsToBeProcessed[i + 2];
let relationshipData = payloadsToBeProcessed[i + 3];

// TODO: maybe delay this allocation slightly?
let inverseRelationshipData = {
data: {
id: id,
type: modelName
}
}
};

// start flushing this individual payload. The logic is the same whether
// it's for the left hand side of the relationship or the right hand side,
Expand Down Expand Up @@ -324,7 +324,7 @@ export default class RelationshipPayloads {
return;
}

if (Array.isArray(data)) {
if (data.length) { // dirty-check for "is-array"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whats wrong with Array.isArray ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend to stick to Array.isArray, which gives you predictable performance, unless you now for sure that data is always an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

// TODO: diff rather than removeall addall?
for (let i=0; i<data.length; ++i) {
this._removeFromInverse(id, data[i].id, inverseIdToPayloads);
Expand Down
Loading