mattphillips / deep-object-diff

Deep diffs two objects, including nested structures of arrays and objects, and returns the difference. ❄️
https://www.npmjs.com/package/deep-object-diff
MIT License
1.05k stars 89 forks source link

Using for arrays? deleted seems to be wrong #43

Closed giorgio79 closed 5 years ago

giorgio79 commented 5 years ago

Tried using it for arrays, but it seems deleted items are tackled incorrectly. The script is not taking the shortest path.

eg https://runkit.com/giorgio79/5bead0643e02a00012ed51d0

const { diff, addedDiff, deletedDiff, detailedDiff, updatedDiff } = require("deep-object-diff");

const wrong = [
 "Tom",
 ",",
 "was",
 "involved",
 "in"
];

const right = [
 "Tom",
 "was",
 "involved",
 "in"
 ];

console.log(detailedDiff(wrong, right));

getting

Object
added: Object {}
deleted: Object {4: undefined}
updated: Object {1: "was", 2: "involved", 3: "in"}

I would have expected Object added: Object {} deleted: Object {1: ","} updated: Object {}

drdreo commented 5 years ago

You are right. Even the documentation example can't distinguish deletes on the same property.

This behavior is actually quiet reasonable. How would you distinguish between an update and a delete? Maybe this is even the desired behavior in some cases (queue data structure).

const lhs = {
  foo: {
    bar: {
      a: ['a', 'b','c'],
      b: 2,
      c: ['x', 'y'],
      e: 100 // deleted
    }
  },
  buzz: 'world'
};

const rhs = {
  foo: {
    bar: {
      a: ['a', 'c'], // index 1 ('b')  deleted
      b: 2, // unchanged
      c: ['x', 'y', 'z'], // 'z' added
      d: 'Hello, world!' // added
    }
  },
  buzz: 'fizz' // updated
};

console.log(detailedDiff(lhs, rhs));

// produces: foo.bar.a.1: 'c' -->  updated
// foo.bar.a.2: undefined -->  deleted
devniel commented 5 years ago

got the same problem. is it a bug, right ?

anko commented 5 years ago

I think this is and should continue to be the intended behaviour.

Reasoning, following @giorgio79's example:

The operation to get from

[ "Tom", ",", "was", "involved", "in" ]

to

[ "Tom", "was", "involved", "in" ]

is to shift everything else up 1 index to cover over the ",".

That matches what the diff you see is telling you:

added: Object {}
deleted: Object {4: undefined} // delete last
updated: Object {1: "was", 2: "involved", 3: "in"} // shift everything up

If it said this,

added: Object {}
deleted: Object {1: ","}
updated: Object {}

that would imply the array should be transformed to

[ "Tom", undefined, "was", "involved", "in" ]

which is a different array!

r3wt commented 5 years ago

anko is correct, this is intended behavior.

mattphillips commented 5 years ago

Gonna close this as it is the correct behaviour as @anko explains above.

Thanks @anko for taking the time to add a clear explanation 😄