Rewrite updateMany
to ensure stable sorting order
#2464
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR:
updateMany
implementation insorted_state_adapter
to ensure that applying updates to fields unrelated to the sort comparator leaves the sorting order unchangedFixes #1853 , fixes #2297
There was also a previous PR at #1860 , but I tackled this fresh and ended up fixing both issues with a simpler rewrite.
The core problem is that the old code always did
delete state.entities[update.id]
, and later re-added it. Later, the sorting code doesconst entities = Object.values(state.entities)
. Since JS engines mostly use insertion order for key iteration, deleting an re-adding an item tostate.entities
caused it to sort after other items that have the same comparator values.Also, deleting the item meant that additional updates for that ID weren't actually applied.
I dropped the somewhat confusing
takeUpdatedModel()
function, and opted to just directly mutate the existing item instead, only deleting it if the ID changed. (This means there's a very minor limitation that if you're trying to apply multiple updates to the same item in one shot, and one of those updates in the middle changes the ID, the later ones won't apply... but that seems like a rare enough edge case to not worry about here.)