equals should skip null and undefined nodes

[roadhump]

There is a common pattern to use ternary operators to create conditions inside JSX templates.

<div>{some ? (<span>Foo</span>) : null}</div>

I expect equals should return true when compare these templates

// first
foo = false;
<div>{some ? (<span>Foo</span>) : null}</div>

// second
<div></div>

But now it expects null value and second template must be only

// first
foo = false;
<div>{some ? (<span>Foo</span>) : null}</div>

// second
<div>{null}</div>

I suppose this comparison should skip null and undefined values in such cases. What do you think?

[lelandrichardson]

@roadhump I agree.

We could extend our nodeEquals method to simply treat null, undefined, false values for this.props.children and for the node itself to all be equal.

This may require couple of caveats...

would we consider <div>{[]}</div> to be equal to <div></div>?

would we consider <div>{[null, null, null]}</div> to be equal to <div></div>?

We may want to consider the pros/cons of each case.

[ljharb]

I'd say an empty array should be the same as null - it's trickier in the second case, but if [] is null then an array of nulls also should be.

[roadhump]

I found out that https://github.com/algolia/react-element-to-jsx-string and https://github.com/bkonkle/jsx-chai works pretty well as replacement of current equals. eact-element-to-jsx-string already handles a lot of edge cases. One missed thing, I suppose, that all functions in props are replaced with stub and will not be checked. But I guess it can be fixed with some option.

So my test case now

import React from 'react';

import chai from 'chai'
import chaiEnzyme from 'chai-enzyme';
import chaiJSX from 'jsx-chai';
chai.use(chaiEnzyme());
chai.use(chaiJSX);

const expect = chai.expect

import {shallow} from 'enzyme';

const ComponentFoo = ({name, onClick}) => (
    <div>
        <em ref="em" onClick={onClick}>{name}</em>
        <strong>{[null]}</strong>
        <ComponentBar name={name} />
    </div>)
const ComponentBar = ({name, onClick}) => (<i>{name}</i>)

describe('Component', () => {

    it('works', () => {

        const onClick = () => alert('click');

        const jsx = (<ComponentFoo name="lol" onClick={onClick} />)

        const dom = shallow(jsx);

        expect(dom.get(0)).to.be.eql(<div><em ref="em" onClick={onClick}>lol</em><strong /><ComponentBar name="lol" /></div>);
        expect(dom.find('ComponentBar').get(0)).to.be.eql(<ComponentBar name="lol" />);
        expect(dom.find('em').get(0)).to.be.eql(<em ref="em" onClick={onClick}>lol</em>);

    })
})

So I suppose, probably it would be easier to replace current equals implementation (and debug) with react-element-to-jsx-string and simple comparison from jsx-chai ?

[lelandrichardson]

I don't want to bring in a dependency to the jsx-string library because I think comparing stringified versions of these structures is fundamentally the wrong approach.

I do think our equals algorithm could be better though. We should handle corner cases with null etc. in a more practical way.

For the most part, I'm comfortable saying that things that react effectively "ignores" do not need to be specified in the case of equality checks. The primary cases being null, false, undefined or an array of those things.

This means the following things are all considered equal:

<div>{null}{"Foo"}</div>
<div>{[null]}{"Foo"}</div>
<div>Foo</div>
<div>{[null, "Foo"]}</div>
<div>{[false, "Foo"]}</div>
<div>{["Foo"]}</div>

[koba04]

This issue has been already fixed.