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

Don't notify relationships with links during initialization #5119

Merged
merged 1 commit into from
Aug 10, 2017
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
7 changes: 5 additions & 2 deletions addon/-private/system/relationships/state/relationship.js
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ export default class Relationship {
this.store._updateRelationshipState(this);
}

updateLink(link) {
updateLink(link, initial) {
Copy link
Contributor

Choose a reason for hiding this comment

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

where is this true ?

Copy link
Member Author

@mmun mmun Aug 9, 2017

Choose a reason for hiding this comment

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

https://github.com/emberjs/data/blob/master/addon/-private/system/relationships/state/create.js#L72

Furthermore, this line and the one in my PR are the only two places that call Relationship#push in the test suite.

Copy link
Contributor

Choose a reason for hiding this comment

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

oooooh, that this arg was already present implies that at some point we'd thought of this case and lost it

heimdall.increment(updateLink);
warn(`You pushed a record of type '${this.internalModel.modelName}' with a relationship '${this.key}' configured as 'async: false'. You've included a link but no primary data, this may be an error in your payload.`, this.isAsync || this.hasData , {
id: 'ds.store.push-link-for-sync-relationship'
Expand All @@ -355,7 +355,10 @@ export default class Relationship {

this.link = link;
this.linkPromise = null;
this.internalModel.notifyPropertyChange(this.key);

if (!initial) {
this.internalModel.notifyPropertyChange(this.key);
}
}

findLink() {
Expand Down
2 changes: 1 addition & 1 deletion addon/-private/system/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -2873,7 +2873,7 @@ function setupRelationships(store, internalModel, data, modelNameToInverseMap) {

if (relationshipRequiresNotification) {
let relationshipData = data.relationships[relationshipName];
relationships.get(relationshipName).push(relationshipData);
relationships.get(relationshipName).push(relationshipData, false);
}

// in debug, assert payload validity eagerly
Expand Down
35 changes: 35 additions & 0 deletions tests/integration/relationships/belongs-to-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1500,3 +1500,38 @@ testInDebug("A belongsTo relationship warns if malformatted data is pushed into
});
}, /expected the data for the book relationship on a <chapter:1> to be in a JSON API format/);
});

test("belongsTo relationship with links doesn't trigger extra change notifications - #4942", function(assert) {
Chapter.reopen({
book: DS.belongsTo({ async: true })
});

run(() => {
env.store.push({
data: {
type: 'chapter',
id: '1',
relationships: {
book: {
data: { type: 'book', id: '1' },
links: { related: '/chapter/1/book' }
}
}
},
included: [{ type: 'book', id: '1' }]
});
});

let chapter = env.store.peekRecord('chapter', '1');
let count = 0;

chapter.addObserver('book', () => {
count++;
});

run(() => {
chapter.get('book');
});

assert.equal(count, 0);
});
31 changes: 31 additions & 0 deletions tests/integration/relationships/has-many-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3334,3 +3334,34 @@ test("deleted records should stay deleted", function(assert) {
);
});
});

test("hasMany relationship with links doesn't trigger extra change notifications - #4942", function(assert) {
run(() => {
env.store.push({
data: {
type: 'book',
id: '1',
relationships: {
chapters: {
data: [{ type: 'chapter', id: '1' }],
links: { related: '/book/1/chapters' }
}
}
},
included: [{ type: 'chapter', id: '1' }]
});
});

let book = env.store.peekRecord('book', '1');
let count = 0;

book.addObserver('chapters', () => {
count++;
});

run(() => {
book.get('chapters');
});

assert.equal(count, 0);
});