Skip to content

Add failing test for deep ordered object comparison. #16

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

Closed
wants to merge 1 commit into from

Conversation

paulirish
Copy link

I wasn't expecting the result I get from d-o-d for this comparison.
Hopefully a test case is more useful to you than a straight bug report. :)

Do the expectations below seem worthwhile?

Note that L70 currently passes (woo!), however L94 and L95 both fail (boo!)

@mattphillips
Copy link
Owner

Hey @paulirish thanks for the failing tests! 😄

The issue you're seeing with the failing tests is because of arrays and their indexes.

If we look at what is happening when you insert into the start of an array (or any index that isn't the tail). The difference isn't that something was added at that index but rather the pre-existing value is updated and everything afterwards is updated until the last item which will now be considered as a new entry even though it was previously in the array.

I find it useful to visualise this with an object (which is all an array actually is).

const original = {
  '0': 'hello',
  '1': 'world',
};

const updated = {
  '0': 'paul',
  '1': 'hello',
  '2': 'world',
};

// a detailed diff would show:
console.log(detailedDiff(original, updated))
/*
{
  added: { '2': 'world' },
  deleted: {},
  updated: { '0': 'paul', '1': 'hello' }
}
*/

As you can see all existing properties of the data structure are being updated when we insert at the start.

This is a common problem I see when performing diffs on arrays (when building d-o-d I held the same assumption). Perhaps there is a way to determine a difference in array in this context. I think the challenge of this is when multiple changes occur (additions/updates/deletions) and potentially at nested paths in the tree.

Do you have any ideas of how this context could be derived?

@mattphillips
Copy link
Owner

Closing for now as this is the expected behaviour. Feel free to reopen.

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

Successfully merging this pull request may close these issues.

2 participants