isShallowEqual: Account for implicit undefined of second object

[aduth]

Fixes #15905

This pull request seeks to resolve an issue in the implementation of isShallowEqualObjects, where an incorrect value of true may be returned in cases where an explicit value of undefined exists in the first, but not second object passed for comparison.

isShallowEqualObjects( { a: undefined }, { b: 5 } )
// Before: `true`
// After: `false`

Implementation Notes:

See #15905 (comment) and #15905 (comment) for additional context.

The implementation makes assumptions that if the length of keys is the same, the values of each object can be compared strictly (looping only using the keys of the first object). However, in the above example, the value of the a key in the second object is implicitly undefined, but it should report false since the key does not exist in the second object.

I have found the changes proposed here to be the best balance of resolving the issue with minimal performance impact. It relegates the edge case to prefer a trivial undefined comparison of the value in the first object before proceeding with the (relatively) expensive Object#hasOwnProperty check. Technically the hasOwnProperty alone is sufficient to resolve the issue, but I had found in benchmarking that it sacrificed more performance than as written here.

Testing Instructions:

Ensure unit tests pass:

npm run test-unit packages/is-shallow-equal

cc @matthewvalentine

[aduth]

The diff makes it look like this was changed, but in fact I had removed this test case altogether as considered redundant with the previous case, which calls isShallowEqual with the objects in both possible arguments order arrangements.

See 100ab09

[jorgefilipecosta]

The change makes logical sense and worked well on some manual sanity checks via wp.isShallowEqual invokations on the browser console.

[jorgefilipecosta]

Unrelated: I'm curious of why while was used here instead of a for (that would be more natural for this loop), was it because "while" proved to be more performant?

[pento]

I've played around a little with loop variations, while seems to be the most performant (in Node, at least).

[aduth]

Probably also worth pointing out that these are very much engine-specific optimizations that I tend to be generally okay with, but also noting that:

• The performance characteristics can (and likely will) change over time
• Ideally they are still equally as performant (or as close to as possible) in other engines
  • In the context of Node at least, optimizing specifically for V8 is reasonable, but in a browser environment, we should be considerate of SpiderMonkey and friends

[pento]

Just for funsies, I tried a few different ways of implementing this.

• Other loop types instead of the while. Eg, for, for...in, etc.
• Several variations of precompiled comparison functions

The loop types were about 97% as fast as the current loop. The best I got out of precompiled functions was about 60% of the current loop.

Interestingly, replacing the loop with aKeys.reduce() gave a 10% improvement for the equal case, but a 10% penalty for the unequal case.

Finally, I did find one thing that seemed to give pretty consistent benefits: swapping i++ for ++i. This is kind of surprising, as V8 should be smart enough to make this optimisation itself, but apparently not in this case. 🤷🏻‍♂️

[pento]

It'd be useful to add a comment here explaining why the comparison is being done like this.

[aduth]

It'd be useful to add a comment here explaining why the comparison is being done like this.

Agree 👍 Added in 741fc5c.

[aduth]

Just for funsies, I tried a few different ways of implementing this.

I'm happy to hear I'm not the only one who finds joy in micro-optimizations 😄

Several variations of precompiled comparison functions

You might enjoy my project turbo-combine-reducers which leverages this very technique (it's used in @wordpress/data).