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

Differences in objects with properties that are associative arrays are not detected #88

Closed
adamlwatson opened this issue Aug 7, 2020 · 8 comments

Comments

@adamlwatson
Copy link

It seems as if associative arrays are used as properties within an object, changes to the values in the keys of those arrays are not being properly.

For example, in the following code, I have two objects, a and b.
In each object, there is a similarly-named property, columns, which is an associative array.
The associative array in each has a single key, foo, which itself contains an object as the value.
This object is populated with a single key in object a, and two keys in object b.
Comparing these two objects results in a true result from isEqual()

image

@adamlwatson adamlwatson changed the title Changes to objects within properties that are associative arrays are not detected Differences in objects with properties that are associative arrays are not detected Aug 7, 2020
@adamlwatson
Copy link
Author

I'm assuming this is because of the odd way that JS returns a length of 0 for associative arrays.... but perhaps this is intended behavior?

@ryan-roemer
Copy link
Member

Can you show us how you are creating those two sample objects in code? And perchance do you know how lodash's isEqual treats them?

@adamlwatson
Copy link
Author

Sure thing... here's how I created a:

image

@adamlwatson
Copy link
Author

Not sure how lodash treats the same... but I will add in the library and attempt the same if you'd like.

@ryan-roemer
Copy link
Member

ryan-roemer commented Aug 7, 2020

Yeah, so I did this experiment, and lodash (which is generally considered the "most correct" although definitely not the fastest deep compare) behaves similarly:

Screen Shot 2020-08-07 at 3 04 26 PM

In RFC's case, we rely on Array.isArray() returning true to allow us to just iterate and compare items by length and index up to length. (See https://github.com/FormidableLabs/react-fast-compare/blob/master/index.js#L18-L24 for full implementation). I'm guessing lodash does something similar, as to accommodate the non-standard use case of manually added properties to an object of type Array would need to be something like:

  1. First, treat as array -- Iterate i up to .length
  2. Second, if all true, treat as object -- iterate Object.keys.

... and that's going to have perf implications that we'd really want to balance with a large user-base need. Perhaps you can chat more about how first-class Arrays are coming up with custom, object-style, fields on them to help frame the discussion better? Thanks!

@adamlwatson
Copy link
Author

Interesting... and also makes sense performance-wise.

For my particular use case, switching from the use of an Array to Object to store individual keyed values solved the problem without any need for major refactoring. This is from an older codebase I'm refactoring, so the use of Arrays in this manner is a side-effect of not-so-clean legacy code.

I think it's still important bring up the issue in case anyone else is experiencing the same issue with code that utilizes Arrays in this manner.

Thank you much for verifying and for the detailed response and explanation!

@simplecommerce
Copy link

simplecommerce commented Jan 5, 2021

Hi, is it possible that the same issue can apply to array of objects that have different property values?

For example:

[
  {
    filterValue: undefined
  }
]
[
  {
    filterValue: "a"
  }
]

I have a case where I am comparing react props and the changes that occur are in a nested array object's property value, it doesn't seem to detect the change, unless I reduce the data to the specific objects.

Could it be a similar issue to the above?

Sorry if I am being unclear.

Update: ok I think I know my issue, its because some props have circular references.

@gksander
Copy link
Contributor

Closing this, as it appears to be resolved/addressed.

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

No branches or pull requests

4 participants