[FEAT BUGFIX] resolves issues with links and data in relationships #5410
Conversation
| localStateIsEmpty() { | ||
| let manyArray = this.manyArray; | ||
| let internalModels = manyArray.currentState; | ||
| let manyIsArrayLoaded = manyArray.get('isLoaded'); |
There was a problem hiding this comment.
s/manyIsArrayLoaded/manyArrayIsLoaded perhaps
There was a problem hiding this comment.
confirm, was dyslexic.
| @@ -77,6 +77,7 @@ export default class Relationship { | |||
| this.meta = null; | |||
| this.hasData = false; | |||
| this.hasLoaded = false; | |||
| this.hasLocalData = false; | |||
There was a problem hiding this comment.
What's the case where hasData && !hasLocalData?
ie how does "local" data differ from data?
Does this capture explicit data in a json-api resource as opposed to data loaded from a link in a json-api resource that initially contained no data?
There was a problem hiding this comment.
hasData really means hasRelationshipDataProperty, it doesn't indicate whether that data has been loaded or not. It's quite possible to have relationshipData and have incomplete data locally.
Example:
{
data: {
type: 'foo',
id: '1',
relationships: {
bars: {
data: [ { type: 'bar', id: '1' } ],
links: { related: './bars' }
}
}
}
}In the above case even though we know which bars the links ought to provide, they have not been given to us in included and so unless they were already available locally we would need to fetch. To cover this, I now do a dirty check for isEmpty to see if we have data.
It might be more clear to update the existing hasData prop to be hasRelationshipDataProperty
There was a problem hiding this comment.
Maybe (probably) because I'm french, but I would (wrongly?) better understand relationshipHasDataProperty.
|
I added 25 new tests to ensure full coverage of the links+data issue. We have 8 failures but I think they should be pretty easy to resolve. |
| let hasData = get(this.hasManyRelationship, 'hasData'); | ||
| if (!hasData) { | ||
| let hasRelationshipDataProperty = get(this.hasManyRelationship, 'hasAnyRelationshipData'); | ||
| if (!hasRelationshipDataProperty) { |
There was a problem hiding this comment.
this should probably be noted for a follow up PR to be changed. No tests failed but now that we've clarified what the flags are it is rather obvious that this is not correct.
| '\nUse `record.belongsTo(relationshipName).meta()` instead.', | ||
| false | ||
| ); | ||
| }), |
There was a problem hiding this comment.
sidenote: this is actually true for hasMany as well. It does proxy meta, however it rarely picks up the change notification (if ever). We should address this soon.
|
Thanks @runspired. |
|
Wow. Nicely done. |
|
Awesome @runspired 🥇 🎉 |
Description
This PR includes two fixes for links and a suite of new tests to accompany them.
The first addresses a shortcoming of the lazy-relationships work which caused
metaandlinksinformation to be lossy. We now "merge forward" meta and links information on relationship payloads when necessary. We also now handle data forhas-manyrelationships discovered via their inverse in a special way (see_partialData). This avoids us losing information about whether the membership should be considered complete or not.The second addresses the situation in which both
dataandlinksare present for a relationship. We now ALWAYS prefer to fetch/re-fetch relationship data via a link if present. To achieve this, we now do a minimal amount of book-keeping on whether the relationship should be considered available locally, or must be refetched.General Warning
This PR cleans up some previously poorly spec'd behavior, which means that it may cause some minor breakage to apps using
linksthat had found work-arounds for themixing links and dataissue.Because it fixes our ability to use
linksfor relationships in general, folks that implemented workarounds for relationships expecting their links to never be used might be particularly surprised. Ideally things will"just work"™in most situations for these folks, and they can delete their workarounds.Resolves and/or supplants
reloadRecordon <xyz> while in state root.loading" when reloading a belongsTo association multiple times simultaneously #5423parent.childrenbecomeshasLoaded: truewhen a child that references the parent is pushed to the store #5211belongsTo#4099It additionally advances the status of #2905 and #2162