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

Add tests for createRecord+unloadRecord #5528

Merged

Conversation

Gabbyjose
Copy link
Contributor

@Gabbyjose Gabbyjose commented Jul 16, 2018

Added tests to cover creating and unloading an owner with associated pet, and creating and unloading a pet with associated owner

see #5472 for motivation

@runspired runspired force-pushed the add-tests-for-create-record-unload-record branch from 99e77aa to 3d6a203 Compare July 16, 2018 21:02
@runspired runspired changed the base branch from add-tests-for-create-record-unload-record to master July 16, 2018 21:02
manyArray.clear();
}
});

Copy link
Member

Choose a reason for hiding this comment

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

Under what circumstances do we have entries in _relationshipPromisesCache or _manyArrayCache but no record?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that should be an invalid state as of now but wouldn't necessarily be so in the future, moved back within the guard for hasRecord.

It is likely in this case instead of retaining we should destroy

- @runspired
*/
Copy link
Member

Choose a reason for hiding this comment

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

this comment can probably be one line

Copy link
Contributor

Choose a reason for hiding this comment

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

Cleaned up this comment a bit by moving a necessary clarifying bit from further up into the comment body.

- @runspired
*/
if (manyArray && !manyArray._inverseIsAsync) {
manyArray.clear();
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 motivation for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sync relationships act as client-side delete. Previously, in this case the manyArray would be cleared by the relationship in this scenario, but with the refactoring for RecordData the manyArray is now managed by InternalModel and thus InternalModel must clear it in this case.

may tell us that this relationship is no longer correct
but that is not enough to tell us that this relationship is
now empty for sure. Likely we should be stale here but
that is probably a breaking change.
Copy link
Member

Choose a reason for hiding this comment

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

Can you rephrase this? what's an example payload that illustrates this?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the simplest setup, foo and bar have a 1:1 relationship.

Payload 1

{
  id: '1',
  type: 'foo',
  relationships: {
    bar: {
      data: { type: 'bar', id: '1' }
    }
  }
},
{
  id: '1',
  type: 'bar',
  relationships: {
    foo: {
      data: { type: 'foo', id: '1' }
    }
  }
}

After receiving payload one we have full knowledge of <foo:1>.bar and <bar:1>.foo.

Payload 2

{
  id: '1',
  type: 'bar',
  relationships: {
    bar: {
      data: { type: 'foo', id: '2' }
    }
  }
}

After receiving payload two, we have full knowledge of <bar:1>.foo and <foo:2>.bar. However, while we are aware that <foo:1>.bar is no longer <bar:1>, we do not know with certainty that the canonical state of the relationship is now null and not some other bar.

Copy link
Member

Choose a reason for hiding this comment

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

However, while we are aware that foo:1.bar is no longer bar:1, we do not know with certainty that the canonical state of the relationship is now null and not some other bar.

What does this have to do with the second payload? It is generally true that when the relationship of some other record changes this tells us nothing about any other relationship besides its inverse.

Copy link
Member

Choose a reason for hiding this comment

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

Put another way, once we sever the connection between <foo:1> and <bar:1> any other information about <bar:1>.foo tells us nothing about <foo:1>.bar unless the value is <foo:1>

Of course something might have changed on the server and our client state could be wrong, but that's always the case. The two flags being set represent client state; in the case you describe client state is still correct, no? Which flag do you think is not correct in this scenario?

pets = chris
.get('pets')
.toArray()
.map(pet => pet.get('name'));
Copy link
Member

Choose a reason for hiding this comment

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

is updating the state of unloaded records a new feature here or have we verified this was 2.12 behaviour?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

ok seems fine although i presume we do not intend to make guarantees about the state of relationships on unloaded records and adding this here is solely a matter of pragmatism.

@runspired runspired changed the title Add tests for create record unload record Add tests for createRecord+unloadRecord Jul 18, 2018
@@ -65,13 +65,13 @@ export default class ModelDataWrapper {
internalModel.notifyPropertyChange(key);
}

notifyHasManyChange(modelName, id, clientId, key) {
notifyHasManyChanged(modelName, id, clientId, key) {
Copy link
Member

Choose a reason for hiding this comment

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

This is public api so we shouldn't change this

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