FormidableLabs / react-fast-compare

fastest deep equal comparison for React
MIT License
1.59k stars 54 forks source link

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

Closed adamlwatson closed 1 year ago

adamlwatson commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

image

adamlwatson commented 4 years ago

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

ryan-roemer commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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 commented 1 year ago

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