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.

Sign up for GitHub

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

[BUGFIX] keep local state after a deletion #5058

Merged
merged 16 commits into from
Jul 14, 2017
Merged

[BUGFIX] keep local state after a deletion #5058

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
879a814
fix failing tests
runspired Jul 10, 2017
269b1b4
fix async relationships
runspired Jul 10, 2017
b972555
expand comment
runspired Jul 10, 2017
4c3d594
add additional test coverage
runspired Jul 10, 2017
e9acc1f
address feedback from @stefanpenner
runspired Jul 11, 2017
941ca58
fix perf of unloading inverses
runspired Jul 11, 2017
9a72fb5
fix typos
runspired Jul 11, 2017
cee07bd
revert accidental code commenting left behind
runspired Jul 11, 2017
94503e6
code cleanup to address @dhamilton review
runspired Jul 11, 2017
4d7f4e8
address remaining feedback from @hjdivad
runspired Jul 11, 2017
5830358
fix type
runspired Jul 11, 2017
e5cdfe4
fixup function naming
stefanpenner Jul 13, 2017
b538bbf
un-diverge isNew and deletion scenarios
runspired Jul 14, 2017
348b72f
improve code comment
runspired Jul 14, 2017
02a5a46
remove unused code
stefanpenner Jul 14, 2017
8c3f77d
final cleanup
runspired Jul 14, 2017
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
31 changes: 23 additions & 8 deletions addon/-private/system/model/internal-model.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,12 +728,6 @@ export default class InternalModel {
}
}

notifyHasManyRemoved(key, record, idx) {
if (this.hasRecord) {
this._record.notifyHasManyRemoved(key, record, idx);
}
}

notifyBelongsToChanged(key, record) {
if (this.hasRecord) {
this._record.notifyBelongsToChanged(key, record);
Expand Down Expand Up @@ -889,13 +883,34 @@ export default class InternalModel {
this.eachRelationship((name, relationship) => {
if (this._relationships.has(name)) {
let rel = this._relationships.get(name);

rel.clear();
rel.removeInverseRelationships();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hjdivad commenting these out didn't make any tests fail o.O

Copy link
Member

Choose a reason for hiding this comment

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

hmm yeah it looks like the unload tests assert against the id map and the relationship payloads, but not against relationships (either our side or inverse).

}
});
Object.keys(this._implicitRelationships).forEach((key) => {
this._implicitRelationships[key].clear();
this._implicitRelationships[key].removeInverseRelationships();
let rel = this._implicitRelationships[key];

rel.clear();
rel.removeInverseRelationships();
});
}

/*
@method unloadDeletedRecordFromRelationships
*/
unloadDeletedRecordFromRelationships() {
Copy link
Member

Choose a reason for hiding this comment

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

per our chat out of band, let's rename this to not conflate this case with unloadRecord

Copy link
Member

Choose a reason for hiding this comment

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

Also let's add some comments here as breadcrumbs to our future selves who will clean this up.

This does fix an important bug, but currently there are two codepaths whose different semantics are not obvious within the same level of abstraction, ie that rel.clear() causes a flushCanonical but unloadDeletedInverseInternalModel does not is not clear, and that this is the entire point is also not clear.

Copy link
Member

Choose a reason for hiding this comment

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

We use unload as the term for unloading records, maybe we should call this method something else, like remove?

Copy link
Member

Choose a reason for hiding this comment

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

Absolutely.

@runspired I guess we missed an instance of the renaming

Copy link
Contributor Author

Choose a reason for hiding this comment

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

confirm, will rename

Copy link
Member

Choose a reason for hiding this comment

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

How about: removeFromInverseRelationships, example:

internalModel.removeFromInverseRelationships();

The method would now describes concisely what is does not when to use it. The when and why should be provided via docs, or derived contextually from where it is used.

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.eachRelationship((name) => {
if (this._relationships.has(name)) {
let rel = this._relationships.get(name);

rel.unloadDeletedInverseInternalModel(this);
}
});
Object.keys(this._implicitRelationships).forEach((key) => {
let rel = this._implicitRelationships[key];

rel.unloadDeletedInverseInternalModel(this);
});
}

Expand Down
2 changes: 1 addition & 1 deletion addon/-private/system/model/states.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,7 +671,7 @@ const RootState = {
isDirty: false,

setup(internalModel) {
internalModel.clearRelationships();
internalModel.unloadDeletedRecordFromRelationships();
Copy link
Member

Choose a reason for hiding this comment

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

internalModel.removeFromInverseRelationships()

},

invokeLifecycleCallbacks(internalModel) {
Expand Down
19 changes: 19 additions & 0 deletions addon/-private/system/relationships/state/belongs-to.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,25 @@ export default class BelongsToRelationship extends Relationship {
this.notifyBelongsToChanged();
}

unloadDeletedInverseInternalModelFromOwn(internalModel) {
Copy link
Member

Choose a reason for hiding this comment

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

foo.set(belongsTo, new);
new.destroyRecord();

foo.get(belongsTo) === null;
``

We should not restore canonical when deleting new; we should let belongsto be null.  That's consistent with [current behaviour](https://ember-twiddle.com/7f94a0e81ea38d9022a5007ef8be15c8?openFiles=adapters.application.js%2C) and in any case, restoring here is a bit wat

super.unloadDeletedInverseInternalModelFromOwn(internalModel);
let didChange = false;

if (this.canonicalState === internalModel) {
this.canonicalState = null;
didChange = true;
}

if (this.inverseInternalModel === internalModel) {
this.inverseInternalModel = this.canonicalState;
didChange = true;
}

if (didChange === true) {
this.notifyBelongsToChanged();
}
}

flushCanonical() {
//temporary fix to not remove newly created records if server returned null.
//TODO remove once we have proper diffing
Expand Down
20 changes: 20 additions & 0 deletions addon/-private/system/relationships/state/has-many.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,26 @@ export default class ManyRelationship extends Relationship {
super.removeCanonicalInternalModelFromOwn(internalModel, idx);
}

unloadDeletedInverseInternalModelFromOwn(internalModel) {
super.unloadDeletedInverseInternalModelFromOwn(internalModel);

const canonicalIndex = this.canonicalState.indexOf(internalModel);

if (canonicalIndex !== -1) {
this.canonicalState.splice(canonicalIndex, 1);
}

const manyArray = this._manyArray;

if (manyArray) {
const idx = manyArray.currentState.indexOf(internalModel);

if (idx !== -1) {
manyArray.internalReplace(idx, 1);
}
}
}

flushCanonical() {
if (this._manyArray) {
this._manyArray.flushCanonical();
Expand Down
30 changes: 30 additions & 0 deletions addon/-private/system/relationships/state/relationship.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
import { assert, warn } from '@ember/debug';
import OrderedSet from '../../ordered-set';
import _normalizeLink from '../../normalize-link';
import Ember from 'ember';

const { guidFor } = Ember;

const {
addCanonicalInternalModel,
Expand Down Expand Up @@ -266,6 +269,33 @@ export default class Relationship {
this.flushCanonicalLater();
}

unloadDeletedInverseInternalModel(internalModel) {
Copy link
Member

Choose a reason for hiding this comment

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

what's the case where internalModel != this.internalModel?

Copy link
Member

Choose a reason for hiding this comment

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

Per our discussion out of band, let's fix the inconsistency in naming here. unloadDeletedInverseInternalModel and unloadDeletedInverseInternalModelFromOwn both mention an "inverse internal model" but are referring to different sides of the relationship.

if (!this.inverseKey) { return; }

// we actually want a union of members and canonicalMembers
// they should be disjoint but currently are not due to a bug
let seen = Object.create(null);

const unload = inverseInternalModel => {
const id = guidFor(inverseInternalModel);

if (seen[id] === undefined) {
const relationship = inverseInternalModel._relationships.get(this.inverseKey);
relationship.unloadDeletedInverseInternalModelFromOwn(internalModel);
seen[id] = true;
}
};

this.members.forEach(unload);
this.canonicalMembers.forEach(unload);
}

unloadDeletedInverseInternalModelFromOwn(internalModel) {
this.canonicalMembers.delete(internalModel);
this.members.delete(internalModel);
this.notifyRecordRelationshipRemoved(internalModel);
Copy link
Member

Choose a reason for hiding this comment

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

does this method do anything? I only see two implementations and they appear to be noops.

https://github.com/emberjs/data/search?utf8=%E2%9C%93&q=notifyRecordRelationshipRemoved&

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this was put here for something with dematerialization that was never completed maybe?

}

flushCanonical() {
heimdall.increment(flushCanonical);
let list = this.members.list;
Expand Down
4 changes: 3 additions & 1 deletion tests/integration/relationships/has-many-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2520,7 +2520,9 @@ test("adding and removing records from hasMany relationship #2666", function(ass
return comments.get('lastObject').destroyRecord();
}).then(() => {
let comments = post.get('comments');
assert.equal(comments.get('length'), 3, "Comments count after destroy");
let length = comments.get('length');

assert.equal(length, 3, "Comments count after destroy");

// Add another comment #4
let comment = env.store.createRecord('comment');
Expand Down
Loading