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

HasMany improvements #442

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
5 changes: 4 additions & 1 deletion packages/ember-model/lib/has_many.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ Ember.hasMany = function(type, options) {
if (!existingArray) {
existingArray = this.getHasMany(options.key || propertyKey, type, meta, this.container);
}
return existingArray.setObjects(newContentArray);
if (!Ember.isEqual(newContentArray, existingArray)) {
return existingArray.setObjects(newContentArray);
}
return existingArray;
}
}).meta(meta);
};
Expand Down
17 changes: 13 additions & 4 deletions packages/ember-model/lib/has_many_array.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,19 @@ Ember.ManyArray = Ember.RecordArray.extend({
},

load: function(content) {
Ember.setProperties(this, {
content: content,
originalContent: content.slice()
});
var currentContent = get(this, 'content');
var mustUpdateCollection = content.length !== currentContent.length;
for (var i = 0, l = content.length; i < l && !mustUpdateCollection; i++) {
var existingItem = currentContent[i];
var newItem = content[i];
mustUpdateCollection = newItem !== existingItem;
}
if (mustUpdateCollection) {
Ember.setProperties(this, {
content: content,
originalContent: content.slice()
});
}
set(this, '_modifiedRecords', []);
},

Expand Down
54 changes: 54 additions & 0 deletions packages/ember-model/tests/has_many/embedded_objects_load_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,57 @@ test("loading embedded data into a parent with deleted children deletes the chil
equal(post.get('comments.length'), 1);
equal(post.get('comments.firstObject.body'), 'new');
});

test("loading collections with same element twice does not trigger observers", function() {
expect(4);
var json = {
id: 1,
title: 'foo',
comments: [{
id: 1,
text: 'helo'
}]
};

var Comment = Ember.Model.extend({
id: attr(),
text: attr()
});
Comment.adapter = Ember.RESTAdapter.create();
Comment.url = '/comments';

var Article = Ember.Model.extend({
title: attr(),
comments: Ember.hasMany(Comment, { key: 'comments', embedded: true }),

commentsChangeCounter: 0,
commentsChanged: function() {
this.incrementProperty('commentsChangeCounter');
}.observes('comments.[]')
});
Article.adapter = Ember.RESTAdapter.create();
Article.url = '/articles';
Article.adapter._ajax = function() {
return new Ember.RSVP.Promise(function(resolve) {
resolve(json);
});
};

var article = Article.create();
Ember.run(article, article.load, json.id, json);
// Force collection to be taken into account (_registerHasManyArray)
var comments = article.get('comments');
equal(article.get('commentsChangeCounter'), 0, "Inital load didn't triggered observers");
var json2 = {
id: 1,
title: 'foo',
comments: [{
id: 1,
text: 'helo here' // <- updated
}]
};
Ember.run(article, article.load, json.id, json2);
equal(article.get('commentsChangeCounter'), 0, "Load with the same collection didn't triggered observers");
equal(comments.get('isDirty'), false, "comments should not be dirty");
deepEqual(Ember.run(comments, comments.mapBy, 'text'), ['helo here']);
});
48 changes: 47 additions & 1 deletion packages/ember-model/tests/has_many/manipulation_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -283,4 +283,50 @@ test("setting a hasMany array with setObjects", function() {

equal(article.get('comments.length'), 3, "should be 3 comments after revert");
equal(article.get('comments.isDirty'), false, "should not be dirty after revert");
});
});

test("setting a has many array with the same array should not trigger its observers", function() {
expect(7);
var json = {
id: 1,
title: 'foo',
comments: [1, 2, 3]
};

var Comment = Ember.Model.extend({
text: attr()
});

var Article = Ember.Model.extend({
title: attr(),

comments: Ember.hasMany(Comment, { key: 'comments' }),

commentsChangeCounter: 0,
commentsChanged: function() {
this.incrementProperty('commentsChangeCounter');
}.observes('comments.[]')
});

Comment.adapter = Ember.FixtureAdapter.create();
Comment.FIXTURES = [
{id: 1, text: 'uno'},
{id: 2, text: 'dos'},
{id: 3, text: 'tres'}
];

var article = Article.create();
Ember.run(article, article.load, json.id, json);

equal(article.get('comments.length'), 3, "should be 3 comments");

article.set('comments', article.get('comments'));
equal(article.get('comments.length'), 3, "should still be 3 comments");
equal(article.get('comments.isDirty'), false, "comments should not be dirty");
equal(article.get('commentsChangeCounter'), 0, "comment observers should not fire");

article.set('comments', [Comment.find(3)]);
equal(article.get('comments.length'), 1, "should be 1 comment after set");
equal(article.get('comments.isDirty'), true, "comments should be dirty after set");
equal(article.get('commentsChangeCounter'), 1, "comment observers should have fired 1 time");
});