equals should skip null and undefined nodes fix #151

[smacker]

#151

[lelandrichardson]

@smacker this looks great as far as i can tell. I get really nervous changing this algorithm though. I'd like to add some more assertions / variations of tests before merging this in to hedge against having a regression.

Thanks for taking a stab at this!

[smacker]

@lelandrichardson what tests would you like to add? Current tests suite of nodeEqual looks solid for me.

[lelandrichardson]

@smacker Apologies for the delay. I want to get this change in, but just want to make sure it's not a regression before we merge. The test coverage right now is good but these equality metrics have a tendency to produce strange corner cases.

I'm going to pull this branch down when I get a chance and play around with adding some tests. Thanks again for putting up a PR!

[smacker]

This test still fail in master branch:

it('should skip null children', () => {
  expect(nodeEqual(
    <div>{null}</div>,
    <div></div>
  )).to.equal(true);
});

It makes me sad :(

[lelandrichardson]

@smacker you're absolutely right. It's my fault - i'm sorry.

I want to get this change in I just wanted to test it more thoroughly first in fear of it causing an unwanted regression.

I will pull this branch down tonight and test it out. Thanks for your patience.

[lelandrichardson]

@smacker I've looked over this a bit and there are some corner cases where this algorithm isn't quite doing what it's supposed to.

The problem is that once we are hitting the children property of the object, we are hard returning the entire function based on whether or not the children props are equal or not. This means that if the equality of the children props is evaluated prior to another prop that is not equal, but children is equal, the function will falsely return true.

This can be demonstrated with the following (somewhat contrived) example:

    expect(nodeEqual(
      <div
        children="Foo"
        a="1"
      />,
      <div
        children="Foo"
        a="2"
      />
    )).to.equal(false);

Even though the a prop of both of these nodes is different, this will return true with your algorithm.

In practice, children is often the last prop because it is defined by react internal if you use JSX, and that means that JS engines will enumerate it last.

Fixing this by only returning false if the children are not equal, and continuing iteration if they are, is the right way to solve this.

In this case though, we run into a separate issue where an object that doesn't define children is not equal to one that does, but children is a non-renderable value (like null or an empty array). In this case we need to change the end of our function to count the prop keys, but essentially ignore children in the count:

What I came up with was this:

export function nodeEqual(a, b) {
  if (a === b) return true;
  if (!a || !b) return false;
  if (a.type !== b.type) return false;
  const left = propsOfNode(a);
  const leftKeys = Object.keys(left);
  const right = propsOfNode(b);
  for (let i = 0; i < leftKeys.length; i++) {
    const prop = leftKeys[i];
    if (prop === 'children') {
      if (!childrenEqual(childrenToArray(left.children), childrenToArray(right.children))) {
        return false;
      }
    } else if (!(prop in right)) {
      return false;
    } else if (right[prop] === left[prop]) {
      // continue;
    } else if (typeof right[prop] === typeof left[prop] && typeof left[prop] === 'object') {
      if (!isEqual(left[prop], right[prop])) {
        return false;
      }
    } else {
      return false;
    }
  }

  if (typeof a !== 'string' && typeof a !== 'number') {
    const leftOffset = ('children' in left) ? 1 : 0;
    const rightOffset = ('children' in right) ? 1 : 0;
    return leftKeys.length - leftOffset === Object.keys(right).length - rightOffset;
  }

  return false;
}

Please feel free to change your PR to use this tweaked algorithm, and I will be happy to accept it and to merge it in. I'll go ahead and do it myself if I don't hear back from you in a couple of days, but I wanted to give you an opportunity to update it since you've done the bulk of the work and have been waiting patiently for me to review it.

[smacker]

Yes, I relied on react behavior. And it's not correct.
But your algorithm is incorrect too.

Consider this example:

        expect(nodeEqual(
          <div></div>,
          <div>child</div>
        )).to.equal(true);

I added tests for all cases and fixed algorithm.

[smacker]

rebased again... @lelandrichardson

[lelandrichardson]

Looks good. Thanks for updating the logic, and for your patience!

[lelandrichardson]

LGTM