-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Don't notify belongsTo changes if nothing changed #4605
Conversation
Looks good |
|
||
module('integration/records/relationship-changes - Relationship changes', { | ||
beforeEach() { | ||
Author = DS.Model.extend({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we define these models at the module level, that way our tests don't have to constantly recreate these classes. This scenario likely doesn't affect the overall test run speed so much, but in aggregate they really do add up
@@ -60,8 +60,10 @@ BelongsToRelationship.prototype.flushCanonical = function() { | |||
if (this.inverseRecord && this.inverseRecord.isNew() && !this.canonicalState) { | |||
return; | |||
} | |||
this.inverseRecord = this.canonicalState; | |||
this.record.notifyBelongsToChanged(this.key); | |||
if (this.inverseRecord !== this.canonicalState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a next improvement, maybe we should also check for equality with current state.
ae37f28
to
eba4aef
Compare
@wecc import setupStore from 'dummy/tests/helpers/store';
import Ember from 'ember';
import DS from 'ember-data';
import { module, test } from 'qunit';
const { run } = Ember;
const { attr, belongsTo, Model, hasMany } = DS;
const Author = Model.extend({
name: attr('string'),
posts: hasMany()
});
const Post = Model.extend({
author: belongsTo()
});
let env;
module('integration/records/relationship-changes - Relationship changes', {
beforeEach() {
env = setupStore({
author: Author,
post: Post
});
},
afterEach() {
run(function() {
env.container.destroy();
});
}
});
test('Calling push with updated belongsTo relationship trigger observer', function(assert) {
assert.expect(1);
let observerCount = 0;
run(function() {
let post = env.store.push({
data: {
type: 'post',
id: '1',
relationships: {
author: {
data: { type: 'author', id: '2' }
}
}
}
});
post.addObserver('author', function() {
observerCount++;
});
env.store.push({
data: {
type: 'post',
id: '1',
relationships: {
author: {
data: { type: 'author', id: '3' }
}
}
}
});
});
assert.equal(observerCount, 1, 'author observer should be triggered once');
});
test('Calling push with same belongsTo relationship does not trigger observer', function(assert) {
assert.expect(1);
let observerCount = 0;
run(function() {
let post = env.store.push({
data: {
type: 'post',
id: '1',
relationships: {
author: {
data: { type: 'author', id: '2' }
}
}
}
});
post.addObserver('author', function() {
observerCount++;
});
env.store.push({
data: {
type: 'post',
id: '1',
relationships: {
author: {
data: { type: 'author', id: '2' }
}
}
}
});
});
assert.equal(observerCount, 0, 'author observer should not be triggered');
});
test('Calling push with non empty hasMany relationship does trigger observer', function(assert) {
assert.expect(1);
let observerCount = 0;
run(function() {
let author = env.store.push({
data: {
id: '2',
type: 'author',
relationships: {
posts: {
data: [{ type: 'post', id: '2' }]
}
}
}
});
author.addObserver('posts', function() {
observerCount++;
});
env.store.push({
data: {
type: 'post',
id: '1',
relationships: {
author: {
data: { type: 'author', id: '2' }
}
}
}
});
});
assert.equal(observerCount, 1, 'posts observer should be triggered');
});
test('Calling push with same hasMany relationship does not trigger observer', function(assert) {
assert.expect(1);
let observerCount = 0;
run(function() {
let author = env.store.push({
data: {
id: '2',
type: 'author',
relationships: {
posts: {
data: [{ type: 'post', id: '2' }]
}
}
}
});
author.addObserver('posts', function() {
observerCount++;
});
env.store.push({
data: {
type: 'post',
id: '2',
relationships: {
author: {
data: { type: 'author', id: '2' }
}
}
}
});
});
assert.equal(observerCount, 1, 'posts observer should not be triggered');
});
test('Calling push with empty hasMany relationship does trigger observer', function(assert) {
assert.expect(1);
let observerCount = 0;
run(function() {
let author = env.store.push({
data: {
id: '2',
type: 'author'
}
});
author.addObserver('posts', function() {
observerCount++;
});
env.store.push({
data: {
type: 'post',
id: '1',
relationships: {
author: {
data: { type: 'author', id: '2' }
}
}
}
});
});
assert.equal(observerCount, 1, 'posts observer should be triggered');
}); after your changes, this case just stopped working :/ |
Sorry guys, this bug is associated with: #4850 |
Fixes #4579