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

Fix the preserve array function with array of objects #44

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

popojargo
Copy link

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' }] };

@coveralls
Copy link

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

@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

@anko

  1. 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}]
}
  1. 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.

  2. 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.

@bdombro
Copy link

bdombro commented Feb 9, 2023

Speaking of which -- why is preserve-array in the repo when it appears unused? And it's also undocumented.

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.

None yet

5 participants