benjamine / jsondiffpatch

Diff & patch JavaScript objects
MIT License
4.8k stars 469 forks source link

Unexpected behaviour when changing element in array #90

Closed sjoerd222888 closed 8 years ago

sjoerd222888 commented 9 years ago

I have a situation where I do not understand the delta being created. If I have the following sitation:

var testObj = {
    collection : [
         "Astronomy",
         "Cosmology",
         "General Relativity",
]
}
// clone for cache
var testObj_cache = JSON.parse(JSON.stringify(testObj));
testObj.collection[0] = "Astrophysics";

The delta created with jsondiffpatch.diff(testObj_cache, testObj) is then given by:

{"collection":{"0":["Astrophysics"],"_t":"a","_0":["Astronomy",0,0]}}

Where I would have expected this:

{"collection":{_t":"a","_0":["Astronomy","Astrophysics"]}}

As we just change a value. Instead the delta looks like adding a value at index 0 with value "Astrophysics" and then remove the value at index 0 with value "Astronomy".

Maybe I don't understand the format correctly. But I think there should not be two statements about the same index in a delta object.

benjamine commented 9 years ago

interesting, I see your point, one reason this is tracked as a remove+add (instead of a modification) right now is that current algorithm tries to detect items moved in arrays. A refinement step will look at array items added and removed and if any pair of values match, it converts that in a "move". I understand what you expected seems really what most people would expect :), I'm not sure although it's worth the change in logic to make it look like that, but I'll leave this open to give it a thought.

sjoerd222888 commented 9 years ago

a simple solution would be to add a post-processing step for each array delta that converts "remove+add" statements to "change" statements. Would this have a negative impact on performance?

As I interpret your delta format I am just ignoring a remove statement if there is an accompanying add statement.

benjamine commented 8 years ago

I think I'll have to preserve current behavior, the reason is removedIndex and insertedIndex, are both zero in this example but they could differ as they mean different things, the removedIndex (the one with an underscore) is the index in previous/left version of the array, and the insertedIndex is the index at the right version of the array, and that could mean a different thing if other items were removed/moved above in the array.

sjoerd222888 commented 8 years ago

Of course if the indexes where different, then it would rather be a remove and add. But when indexes are the same it could easily be implemented as a change I would think. The payload would be a little smaller and maybe also generating/applying the diff would be a little faster if you create pure change diffs.

But it works as it is. Maybe add this as comment in the documentation?