Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

just-diff considering array positions #505

Closed
talski opened this issue Sep 30, 2022 · 5 comments · Fixed by #541
Closed

just-diff considering array positions #505

talski opened this issue Sep 30, 2022 · 5 comments · Fixed by #541
Assignees
Labels

Comments

@talski
Copy link

talski commented Sep 30, 2022

let original = ['B', 'C', 'D']
let updated = ['A', 'B', 'C', 'D']

console.log(jd.diff(updated, original))

Will result in

[
  { op: 'remove', path: [ 3 ] },
  { op: 'replace', path: [ 0 ], value: 'B' },
  { op: 'replace', path: [ 1 ], value: 'C' },
  { op: 'replace', path: [ 2 ], value: 'D' }
]

There's a way to get this? since we are removing first position, there's no need to replace the other positions

[
  { op: 'remove', path: [ 0 ] }
]

I need to store the steps to get back in time with an object, and the current result will get pretty large as array gets bigger

@qraynaud
Copy link

qraynaud commented Oct 5, 2022

I have the same issue. This is even more true with complex objects that gets completely moved from one cell to others.

const a = { val: 0 };
const b = { val: 1 };
const c = { val: 2 };
console.log(diff([a, b, c], [b, c]));

Results in:

[
  { op: 'remove', path: [ 2 ] },
  { op: 'replace', path: [ 0, 'val' ], value: 1 },
  { op: 'replace', path: [ 1, 'val' ], value: 2 }
]

This is horrible even on small arrays with "big" objects in them.

@angus-c
Copy link
Owner

angus-c commented Nov 19, 2022

Thanks yes this needs optimizing. Apologies I don't have much free time for this project right now but will get to it if someone else does not first.

@angus-c angus-c self-assigned this Nov 19, 2022
@luisdralves
Copy link

Same issue here, I think it may be worth looking into https://github.com/dherault/array-differences. I did some quick tinkering and it is simple enough to integrate and get the desired result. Of course, adding this dependency would defeat the point of being dependency-free, but I think there is value in taking a look at their approach and implementing something similar

These are the changes I made to the code to make use of array-differences:

(this lib uses a similar structure with [ op, path, value ] instead of an object, and the op names are different)

  const opsMap = {
    'modified': 'replace',
    'inserted': 'add',
    'deleted': 'remove'
  }

  function getDiff(obj1, obj2, basePath, diffs) {
    // ...

    if (Array.isArray(obj1) && Array.isArray(obj2)) {
      arrayDifferences.default(obj1, obj2).forEach(diff => {
        const op = opsMap[diff[0]]
        path = basePath.concat(diff[1])
        diffs[op].push({
          op,
          path: pathConverter(path),
          ...(op !== 'remove' && { value:diff[2] })
        })
      })
    } else {
      // the rest of the code
    }
    return diffs;
  }

Note that this doesn't take into consideration arrays of objects or arrays of arrays.

@angus-c
Copy link
Owner

angus-c commented Feb 8, 2023

Ok I have a messy solution. Will work on cleaning it up. Basically for arrays of different lengths we just need to compare from left and right and accept the shorter of the two solutions,

Will try and get sthg done this weekend (but cannot promise)

@angus-c
Copy link
Owner

angus-c commented Mar 12, 2023

This should be finally done (added additional optimization to replace at root, instead of in a nested fashion if values are of different type)
I've added a lot more tests but releasing as a major version because
a) Just in case I messed up
b) Some existing uers may be relying on de-optimized diffs that were previously generated

just-diff@6.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants