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

[BUGFIX release] relationship [x, y] should not break on x.unloadRecord() #5092

Merged
merged 3 commits into from
Jul 25, 2017

Conversation

stefanpenner
Copy link
Member

@stefanpenner stefanpenner commented Jul 25, 2017

For an async relationship [x, y] with x.unloadRecord(), now adjusts only the relationship’s currentState, leaving that relationship’s canonical state alone, ensuring the existing client-side delete semantics are preserved. But when that relationship is reloaded, the canonicalState consulted.

For sync relationship [x, y] with x.unloadRecord(), both currentState and canonical state are updated. This is to mirror the client-side delete semantics. But since we cannot reload a sync relationship we must assume this to be the new canonical state and rely on subsequent push or adapterPayloads or manual store.push to update.

This aims to:

  • [FIX] hasMany arrays never contain dematerialized records (so they no longer become broken)
  • [FIX] using unloadRecord as a type of client side delete
  • [PRESERVE] the garbage collector pass to cleanup orphaned models
  • [PRESERVE] second access to a relationship which did contain an unloadRecord to cause a reload

note: if both sides of a relationships are unloaded, the above doesn’t apply. This is largely just when members of a loaded relationship are themselves unloaded.

[fixes #4986 #5052 #4987 #4996]

@stefanpenner
Copy link
Member Author

@sudowork I have included your failing test (to ensure this does address the issue)

@stefanpenner
Copy link
Member Author

@workmanw I'm not sure if this also addresses #5025 it may not, as that sounds like its in the land of record array manager. If you can provide a failing test, I can take a look.

@stefanpenner stefanpenner mentioned this pull request Jul 25, 2017
@rwjblue
Copy link
Member

rwjblue commented Jul 25, 2017

👍 - This seems like a good path forward to me...

@@ -1645,6 +1727,37 @@ testInDebug('A sync hasMany errors out if there are unlaoded records in it', fun
}, /You looked up the 'comments' relationship on a 'post' with id 1 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 }\)'\)/);
});

testInDebug('After removing and unloading a record, a hasMany relationship should still be valid', function(assert) {
Copy link
Member

Choose a reason for hiding this comment

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

why testInDebug vs test?

run(() => {
cars.get('firstObject').unloadRecord();
// assert.equal(cars.get('length'), 1); // unload now..
// assert.equal(person.get('cars.length'), 1); // unload now..
Copy link
Member

Choose a reason for hiding this comment

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

dead code

Ember.run(() => {
tag.unloadRecord();
assert.equal(people.get('isDestroying'), true);
assert.equal(people.get('isDestroyed'), false);
assert.equal(people.isDestroying, false, 'people is destroying sync after unloadRecord');
Copy link
Member

Choose a reason for hiding this comment

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

assertion msg inverted

assert.equal(people.get('isDestroyed'), false);
assert.equal(people.isDestroying, false, 'people is destroying sync after unloadRecord');
assert.equal(people.isDestroyed, false, 'people is NOT YET destroyed sync after unloadRecord');
assert.equal(peopleProxy.isDestroying, false, 'peopleProxy is destroying sync after unloadRecord');
Copy link
Member

Choose a reason for hiding this comment

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

assertion msg inverted

// assert.equal(person.get('cars.length'), 1); // unload now..
});

// assert.equal(cars.get('length'), 1); // unload now..
Copy link
Member

Choose a reason for hiding this comment

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

dead code

sudowork and others added 2 commits July 25, 2017 14:01
…rd()

For an async relationship [x, y] with x.unloadRecord(), now adjusts only
the relationship’s currentState, leaving that relationship’s canonical
state alone, ensuring the existing client-side delete semantics are
preserved. But when that relationship is reloaded, the canonicalState
consulted.

For sync relationship [x, y] with x.unloadRecord(), both currentState
and canonical state are updated. This is to mirror the client-side
delete semantics. But since we cannot reload a sync relationship we must
assume this to be the new canonical state and rely on subsequent `push`
or `adapterPayloads` or manual `store.push` to update.

This aims to:

* [FIX] hasMany arrays never contain dematerialized records (so they no longer become broken)
* [FIX] using unloadRecord as a type of client side delete is restored
* [PRESERVE] the garbage collector pass to cleanup orphaned models
* [PRESERVE] second access to a relationship which did contain an unloadRecord to cause a reload

note: if both sides of a relationships are unloaded, the above doesn’t
apply. This is largely just when members of a loaded relationship are
themselves unloaded.

[fixes #4986 #5052 #4987 #4996]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants