[BUGFIX FEAT] Preserve local hasMany changes during flushCanonical #5269
Conversation
|
Kicked this off, it's still failing. I'm wondering if it wouldn't be best with these "long term goal" tests to merge them and skip them. If testem would work with |
|
This test may be fixed by #4852 |
When new data comes in via `store.push` for a many array, and `ManyArray.prototype.flushCanonical` is invoked: - new, unpersisted records added to the relationship are correctly not discarded, but - persisted records added to the relationship are erroneously discarded
47eef06 to
43e9a70
Compare
| if (diff.firstChangeIndex === null) { | ||
| // no changes found | ||
| if (removalSet) { | ||
| // records that have been removed since last compute will need their inverses to be corrected |
There was a problem hiding this comment.
aren't the local inverses updated synchronously when the record is initially removed?
| return set; | ||
| } | ||
|
|
||
| export function computeChanges(oldArray, newArray, removalSet) { |
There was a problem hiding this comment.
let's document the semantics of this function
There was a problem hiding this comment.
speaking of which we should probably not have it in diff-array as it's semantics are more specific than array diffing
if we don't want to keep it in relationships/state/has-many maybe move it to something like relationships/state/utils and perhaps rename it to computeHasManyChanges(previousCanonicalState, newCanonicalState, locallyRemoved)
| return set; | ||
| } | ||
|
|
||
| export function computeChanges(oldArray, newArray, removalSet) { |
There was a problem hiding this comment.
let's also add a test for non-contiguous changes
| return changeset; | ||
| } | ||
|
|
||
| let removedMembersSet = setForArray(oldArray.slice(diff.firstChangeIndex, diff.firstChangeIndex + diff.removedCount)); |
There was a problem hiding this comment.
Is this actually the set of removed members or rather the range (as a set) that contains all removed members?
Since we scan the range it's not a correctness issue but it makes the code more confusing
| } | ||
|
|
||
| let removedMembersSet = setForArray(oldArray.slice(diff.firstChangeIndex, diff.firstChangeIndex + diff.removedCount)); | ||
| let changeBlockSet = setForArray(newArray.slice(diff.firstChangeIndex, diff.firstChangeIndex + diff.addedCount)); |
There was a problem hiding this comment.
What is the change block? Isn't this the minimal contiguous range that includes all the added records?
I'm trying to see why the name isn't symmetrical with removedMembersSet
| } | ||
| }); | ||
|
|
||
| let flushCanonicalLater = false; |
There was a problem hiding this comment.
when is this var changed?
The PR is green; if we don't need the local changes we'll still want the tests i think |
|
Closing this in favor of a new attempt at a later point that takes into consideration |
|
Actually, going to reopen this one and PR the |
|
Closing since stale, likely mostly addressed with #7493 |
resolves #5269
When new data comes in via
store.pushfor a many array, andManyArray.prototype.flushCanonicalis invoked:paired with @rwjblue, fix and additional test by @runspired