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

make DS.ManyArray lazy #4600

Merged
merged 1 commit into from
Oct 21, 2016
Merged
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
8 changes: 4 additions & 4 deletions addon/-private/system/many-array.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, {
init() {
this._super(...arguments);
this.currentState = Ember.A([]);
this.flushCanonical();
},

record: null,
Expand All @@ -74,19 +75,18 @@ export default Ember.Object.extend(Ember.MutableArray, Ember.Evented, {
},

flushCanonical() {
//TODO make this smarter, currently its plenty stupid
var toSet = this.canonicalState.filter((internalModel) => !internalModel.isDeleted());
let toSet = this.canonicalState;

//a hack for not removing new records
//TODO remove once we have proper diffing
var newRecords = this.currentState.filter(
let newRecords = this.currentState.filter(
// only add new records which are not yet in the canonical state of this
// relationship (a new record 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(newRecords);
var oldLength = this.length;
let oldLength = this.length;
this.arrayContentWillChange(0, this.length, toSet.length);
// It’s possible the parent side of the relationship may have been unloaded by this point
if (_objectIsAlive(this)) {
Expand Down
16 changes: 9 additions & 7 deletions addon/-private/system/references/has-many.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ import {

import isEnabled from 'ember-data/-private/features';

const get = Ember.get;
const {
RSVP: { resolve },
get
} = Ember;

var HasManyReference = function(store, parentInternalModel, hasManyRelationship) {
this._super$constructor(store, parentInternalModel);
Expand Down Expand Up @@ -45,11 +48,11 @@ HasManyReference.prototype.ids = function() {
};

HasManyReference.prototype.meta = function() {
return this.hasManyRelationship.manyArray.meta;
return this.hasManyRelationship.meta;
};

HasManyReference.prototype.push = function(objectOrPromise) {
return Ember.RSVP.resolve(objectOrPromise).then((payload) => {
return resolve(objectOrPromise).then((payload) => {
var array = payload;
Copy link
Member

Choose a reason for hiding this comment

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

should likely return if the HasManyReference is destroyed by now (assuming it can be destroyed)

Copy link
Member Author

Choose a reason for hiding this comment

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

We have eventually the same problem with belongsTo. I think it's best to create an issue and make a separate PR to fix it (if it's really a problem). I have not changed anything significant in this PR regarding references.

Copy link
Member

Choose a reason for hiding this comment

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

sure, open an issue and link back here, so we can address.


if (isEnabled("ds-overhaul-references")) {
Expand Down Expand Up @@ -102,7 +105,7 @@ HasManyReference.prototype.push = function(objectOrPromise) {

this.hasManyRelationship.computeChanges(internalModels);

return this.hasManyRelationship.manyArray;
return this.hasManyRelationship.getManyArray();
});
};

Expand All @@ -122,7 +125,7 @@ HasManyReference.prototype._isLoaded = function() {

HasManyReference.prototype.value = function() {
if (this._isLoaded()) {
return this.hasManyRelationship.manyArray;
return this.hasManyRelationship.getManyArray();
}

return null;
Expand All @@ -133,8 +136,7 @@ HasManyReference.prototype.load = function() {
return this.hasManyRelationship.getRecords();
}

var manyArray = this.hasManyRelationship.manyArray;
return Ember.RSVP.resolve(manyArray);
return resolve(this.hasManyRelationship.getManyArray());
};

HasManyReference.prototype.reload = function() {
Expand Down
82 changes: 50 additions & 32 deletions addon/-private/system/relationships/state/has-many.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,29 +10,40 @@ export default function ManyRelationship(store, record, inverseKey, relationship
this._super$constructor(store, record, inverseKey, relationshipMeta);
this.belongsToType = relationshipMeta.type;
this.canonicalState = [];
this.manyArray = ManyArray.create({
canonicalState: this.canonicalState,
store: this.store,
relationship: this,
type: this.store.modelFor(this.belongsToType),
record: record
});
this.isPolymorphic = relationshipMeta.options.polymorphic;
this.manyArray.isPolymorphic = this.isPolymorphic;
}

ManyRelationship.prototype = Object.create(Relationship.prototype);
ManyRelationship.prototype.getManyArray = function() {
if (!this._manyArray) {
this._manyArray = ManyArray.create({
canonicalState: this.canonicalState,
store: this.store,
relationship: this,
type: this.store.modelFor(this.belongsToType),
record: this.record,
meta: this.meta,
isPolymorphic: this.isPolymorphic
});
}
return this._manyArray;
};

ManyRelationship.prototype.constructor = ManyRelationship;
ManyRelationship.prototype._super$constructor = Relationship;

ManyRelationship.prototype.destroy = function() {
this.manyArray.destroy();
if (this._manyArray) {
this._manyArray.destroy();
}
};

ManyRelationship.prototype._super$updateMeta = Relationship.prototype.updateMeta;
ManyRelationship.prototype.updateMeta = function(meta) {
this._super$updateMeta(meta);
this.manyArray.set('meta', meta);
if (this._manyArray) {
this._manyArray.set('meta', meta);
}
};

ManyRelationship.prototype._super$addCanonicalRecord = Relationship.prototype.addCanonicalRecord;
Expand All @@ -54,7 +65,8 @@ ManyRelationship.prototype.addRecord = function(record, idx) {
return;
}
this._super$addRecord(record, idx);
this.manyArray.internalAddRecords([record], idx);
// make lazy later
this.getManyArray().internalAddRecords([record], idx);
};

ManyRelationship.prototype._super$removeCanonicalRecordFromOwn = Relationship.prototype.removeCanonicalRecordFromOwn;
Expand All @@ -74,7 +86,9 @@ ManyRelationship.prototype.removeCanonicalRecordFromOwn = function(record, idx)

ManyRelationship.prototype._super$flushCanonical = Relationship.prototype.flushCanonical;
ManyRelationship.prototype.flushCanonical = function() {
this.manyArray.flushCanonical();
if (this._manyArray) {
this._manyArray.flushCanonical();
}
this._super$flushCanonical();
};

Expand All @@ -84,11 +98,12 @@ ManyRelationship.prototype.removeRecordFromOwn = function(record, idx) {
return;
}
this._super$removeRecordFromOwn(record, idx);
let manyArray = this.getManyArray();
if (idx !== undefined) {
//TODO(Igor) not used currently, fix
this.manyArray.currentState.removeAt(idx);
manyArray.currentState.removeAt(idx);
} else {
this.manyArray.internalRemoveRecords([record]);
manyArray.internalRemoveRecords([record]);
}
};

Expand All @@ -99,23 +114,24 @@ ManyRelationship.prototype.notifyRecordRelationshipAdded = function(record, idx)
};

ManyRelationship.prototype.reload = function() {
var manyArrayLoadedState = this.manyArray.get('isLoaded');
let manyArray = this.getManyArray();
let manyArrayLoadedState = manyArray.get('isLoaded');

if (this._loadingPromise) {
if (this._loadingPromise.get('isPending')) {
return this._loadingPromise;
}
if (this._loadingPromise.get('isRejected')) {
this.manyArray.set('isLoaded', manyArrayLoadedState);
manyArray.set('isLoaded', manyArrayLoadedState);
}
}

if (this.link) {
this._loadingPromise = promiseManyArray(this.fetchLink(), 'Reload with link');
return this._loadingPromise;
} else {
this._loadingPromise = promiseManyArray(this.store.scheduleFetchMany(this.manyArray.toArray()).then(() => {
return this.manyArray;
this._loadingPromise = promiseManyArray(this.store.scheduleFetchMany(manyArray.toArray()).then(() => {
return manyArray;
}), 'Reload with ids');
return this._loadingPromise;
}
Expand Down Expand Up @@ -158,27 +174,28 @@ ManyRelationship.prototype.fetchLink = function() {
}
this.store._backburner.join(() => {
this.updateRecordsFromAdapter(records);
this.manyArray.set('isLoaded', true);
this.getManyArray().set('isLoaded', true);
});
return this.manyArray;
return this.getManyArray();
});
};

ManyRelationship.prototype.findRecords = function() {
let manyArray = this.manyArray.toArray();
let internalModels = new Array(manyArray.length);
let manyArray = this.getManyArray()
let array = manyArray.toArray();
let internalModels = new Array(array.length);

for (let i = 0; i < manyArray.length; i++) {
internalModels[i] = manyArray[i]._internalModel;
for (let i = 0; i < array.length; i++) {
internalModels[i] = array[i]._internalModel;
}

//TODO CLEANUP
return this.store.findMany(internalModels).then(() => {
if (!this.manyArray.get('isDestroyed')) {
if (!manyArray.get('isDestroyed')) {
//Goes away after the manyArray refactor
this.manyArray.set('isLoaded', true);
manyArray.set('isLoaded', true);
}
return this.manyArray;
return manyArray;
});
};
ManyRelationship.prototype.notifyHasManyChanged = function() {
Expand All @@ -187,6 +204,7 @@ ManyRelationship.prototype.notifyHasManyChanged = function() {

ManyRelationship.prototype.getRecords = function() {
//TODO(Igor) sync server here, once our syncing is not stupid
let manyArray = this.getManyArray();
if (this.isAsync) {
var promise;
if (this.link) {
Expand All @@ -199,18 +217,18 @@ ManyRelationship.prototype.getRecords = function() {
promise = this.findRecords();
}
this._loadingPromise = PromiseManyArray.create({
content: this.manyArray,
content: manyArray,
promise: promise
});
return this._loadingPromise;
} else {
assert("You looked up the '" + this.key + "' relationship on a '" + this.record.type.modelName + "' with id " + this.record.id + " but some of the associated records were not loaded. Either make sure they are all loaded together with the parent record, or specify that the relationship is async (`DS.hasMany({ async: true })`)", this.manyArray.isEvery('isEmpty', false));
assert("You looked up the '" + this.key + "' relationship on a '" + this.record.type.modelName + "' with id " + this.record.id + " but some of the associated records were not loaded. Either make sure they are all loaded together with the parent record, or specify that the relationship is async (`DS.hasMany({ async: true })`)", manyArray.isEvery('isEmpty', false));

//TODO(Igor) WTF DO I DO HERE?
if (!this.manyArray.get('isDestroyed')) {
this.manyArray.set('isLoaded', true);
if (!manyArray.get('isDestroyed')) {
manyArray.set('isLoaded', true);
}
return this.manyArray;
return manyArray;
}
};

Expand Down
12 changes: 9 additions & 3 deletions tests/integration/adapter/rest-adapter-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -291,9 +291,12 @@ test("createRecord - findMany doesn't overwrite owner", function(assert) {
run(function() {
comment = store.createRecord('comment', { name: "The Parley Letter" });
});
post.get('comments').pushObject(comment);

assert.equal(comment.get('post'), post, "the post has been set correctly");
run(function() {
post.get('comments').pushObject(comment);

assert.equal(comment.get('post'), post, "the post has been set correctly");
});

run(function() {
comment.save().then(assert.wait(function(comment) {
Expand Down Expand Up @@ -482,7 +485,10 @@ test("createRecord - a record on the many side of a hasMany relationship should
});

var post = store.peekRecord('post', 1);
var commentCount = post.get('comments.length');
var commentCount = run(function() {
return post.get('comments.length');
});

assert.equal(commentCount, 1, "the post starts life with a comment");

run(function() {
Expand Down
13 changes: 9 additions & 4 deletions tests/integration/record-array-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -264,11 +264,14 @@ test('a loaded record is not removed from a record array when it is deleted even
});
scumbag = store.peekRecord('person', 1);
tag = store.peekRecord('tag', 1);

scumbag.deleteRecord();
});

assert.equal(tag.get('people.length'), 1, 'record is not removed from the record array');
assert.equal(tag.get('people').objectAt(0), scumbag, 'tag still has the scumbag');
run(function() {
assert.equal(tag.get('people.length'), 1, 'record is not removed from the record array');
assert.equal(tag.get('people').objectAt(0), scumbag, 'tag still has the scumbag');
});
});

test("a loaded record is not removed from both the record array and from the belongs to, even if the belongsTo side isn't defined", function(assert) {
Expand Down Expand Up @@ -320,8 +323,10 @@ test("a loaded record is not removed from both the record array and from the bel
tool = store.peekRecord('tool', 1);
});

assert.equal(tag.get('people.length'), 1, 'record is in the record array');
assert.equal(tool.get('person'), scumbag, 'the tool belongs to the record');
run(function() {
assert.equal(tag.get('people.length'), 1, 'record is in the record array');
assert.equal(tool.get('person'), scumbag, 'the tool belongs to the record');
});

run(() => scumbag.deleteRecord());

Expand Down
10 changes: 6 additions & 4 deletions tests/integration/references/has-many-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -530,10 +530,12 @@ test("value() returns the referenced records when all records are loaded", funct
env.store.push({ data: { type: 'person', id: 2, attributes: { name: "Michael" } } });
});

var personsReference = family.hasMany('persons');
var records = personsReference.value();
assert.equal(get(records, 'length'), 2);
assert.equal(records.isEvery('isLoaded'), true);
run(function() {
var personsReference = family.hasMany('persons');
var records = personsReference.value();
assert.equal(get(records, 'length'), 2);
assert.equal(records.isEvery('isLoaded'), true);
});
});

test("load() fetches the referenced records", function(assert) {
Expand Down
3 changes: 2 additions & 1 deletion tests/integration/relationships/has-many-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1678,8 +1678,9 @@ test("If reordered hasMany data has been pushed to the store, the many array ref
}
});
post = env.store.peekRecord('post', 1);

assert.deepEqual(post.get('comments').toArray(), [comment1, comment2], 'Initial ordering is correct');
});
assert.deepEqual(post.get('comments').toArray(), [comment1, comment2], 'Initial ordering is correct');

run(function() {
env.store.push({
Expand Down
Loading