Fix the preserve array function with array of objects #44

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

Jump to bottom

Open

popojargo wants to merge 1 commit into mattphillips:main

from popojargo:fix/preserve-array

Open

Fix the preserve array function with array of objects #44

popojargo wants to merge 1 commit into mattphillips:main from popojargo:fix/preserve-array

Conversation

popojargo

Copy link

@popojargo popojargo commented Apr 20, 2019

Your examples with works fine execpt when the values compared in the array are objects.

For example. this will work fine:

// added (end of array)
const left = { alphabet: ['a', 'b', 'c'] };
const right = { alphabet: ['a', 'b', 'c', 'd'] };
diff(left, right);
/*
{
 alphabet: [empty, empty, empty, 'd']
}
*/

But, if you use a similar representation with objects, it fails:

const left = { alphabet: [{ a: 'a' }, { b: 'b' }, { c: 'c' }] };
const right = { alphabet: [{ a: 'a' }, { b: 'b' }, { c: 'c' }, { d: 'd' }] };

@popojargo


 Fix the preserve array function with array of objects

0a40800

@coveralls

Copy link

coveralls commented Apr 20, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 0a40800 on popojargo:fix/preserve-array into 6296889 on mattphillips:master.

@popojargo

Copy link

Author

popojargo commented Apr 27, 2019

@mattphillips Can you take a look a this?

@anko

Copy link

Contributor

anko commented Apr 29, 2019

Hi, user here. Drivebying with a lightning review.

Things that made me think:

Could you describe in English what exactly is wrong that you're attempting to fix?
What exactly does this failure you mention look like? You've posted code to replicate it, but I don't know what I'm looking at, or what the output is, or what you expected the output to be.
Your changes only modify existing tests. Perhaps there should be a test for this case specifically?

Hopefully this is helpful 🏃‍♂️

@popojargo

Copy link

Author

popojargo commented Apr 29, 2019

@anko

I'm using the "preserve-array" functions that is available in "src/preserve-array.js". I'm not sure if it's officially suppported but that's what the function that I need to diff my objects.

Basically, I'm using CouchDB has a database. When updating a document, I need to do a partial update in order to update only the fields that have changed. When manipulating objects with diff, it works fine. Although, the array handling by default is not very good.

For this reason, I use preserve-array.js which keeps the array and produce a real "array difference".

So let's say I had originally this document:

{
 "children": [{"name":1}]
}

and I end up with this document:

{
 "children": [{"name":1},{"name":2}]
}

I would like the following diff to apply to the latest document:

{
 "children": [empty,{"name":2}]
}

Using the "preserve-array" works fine with array of primitives (number, string). When I'm using arrays of objects, the code just crash since the lhs or rhs can be null at some point.
Yes I did change the tests but the changes I made are testing the object diff. Since a lot of "test cases" were covered in a single test case, I simply added the new things to cover. I can make it more explicit if required.