unexpected-react icon indicating copy to clipboard operation
unexpected-react copied to clipboard

With react 16 <null> elements are breaking expectation

Open Raigen opened this issue 6 years ago • 9 comments

After the upgrade to react 16.3 and unexpected-react 5 some of our tests are breaking that where working before. In some cases there are strange <null> elements wrapping arrays of elements and the new Fragment. I was able to reproduce the issue partly with the following test case:

    const ListItem = ({ children }) => React.cloneElement(children)
    ListItem.propTypes = { children: any }

    class List extends React.Component {
      static propTypes = { children: any }
      render() {
        return this.props.children()
      }
    }
    const renderList = () => (
      <List>
        {() => (
          <ol>
            {['One', 'Two'].map(t => (
              <ListItem key={t}>
                <li>{t}</li>
              </ListItem>
            ))}
          </ol>
        )}
      </List>
    )

    const renderOne = bool =>
      bool ? (
        <React.Fragment>
          <h4>Headline 1</h4>
          {renderList()}
        </React.Fragment>
      ) : null

    class Component extends React.Component {
      render() {
        return (
          <div>
            <div>
              {/* true ? [<h4 key="1">Headline 1</h4>, renderList()] : null */}
              {/* renderOne(true) */}
              {true ? (
                <React.Fragment>
                  <h4>Headline 1</h4>
                  {renderList()}
                </React.Fragment>
              ) : null}
              <h4>Headline 2</h4>
              {renderList()}
              <h4>Headline 3</h4>
              {renderList()}
            </div>
          </div>
        )
      }
    }
  it('should render', function() {
    const dom = TestUtils.renderIntoDocument(<Component />)

    expect(
      dom,
      'to have rendered',
      <div>
        <div>
          <h4>Headline 1</h4>
          <ol>
            <li>One</li>
            <li>Two</li>
          </ol>
          <h4>Headline 2</h4>
          <ol>
            <li>One</li>
            <li>Two</li>
          </ol>
          <h4>Headline 3</h4>
          <ol>
            <li>One</li>
            <li>Two</li>
          </ol>
        </div>
      </div>,
    )
  })
    UnexpectedError:
    expected
    <Component>
      <div>
        <div>
          <null>
            <h4>Headline 1</h4>
            <List>
              <ol><ListItem><li>One</li></ListItem><ListItem><li>Two</li></ListItem></ol>
            </List>
          </null>
          <h4>Headline 2</h4>
          <List>
            <ol><ListItem><li>One</li></ListItem><ListItem><li>Two</li></ListItem></ol>
          </List>
          <h4>Headline 3</h4>
          <List>
            <ol><ListItem><li>One</li></ListItem><ListItem><li>Two</li></ListItem></ol>
          </List>
        </div>
      </div>
    </Component>
    to have rendered
    <div>
      <div>
        <h4>Headline 1</h4>
        <ol><li>One</li><li>Two</li></ol>
        <h4>Headline 2</h4>
        <ol><li>One</li><li>Two</li></ol>
        <h4>Headline 3</h4>
        <ol><li>One</li><li>Two</li></ol>
      </div>
    </div>

    <Component>
      <div>
        <div>
          // missing <h4>Headline 1</h4>
          // missing <ol><li>One</li><li>Two</li></ol>
          <null>
            <h4>Headline 1</h4>
            <List>
              <ol><ListItem><li>One</li></ListItem><ListItem><li>Two</li></ListItem></ol>
            </List>
          </null>
          <h4>Headline 2</h4>
          <List>
            <ol>
              <ListItem>
                <li>One</li>
              </ListItem>
              <ListItem>
                <li>Two</li>
              </ListItem>
            </ol>
          </List>
          <h4>Headline 3</h4>
          <List>
            <ol>
              <ListItem>
                <li>One</li>
              </ListItem>
              <ListItem>
                <li>Two</li>
              </ListItem>
            </ol>
          </List>
        </div>
      </div>
    </Component>

When I add the null element to the expected structure I get <null // should be <null. When I add React.Fragment to the expected structure I get <null // should be <no-display-name. It does not matter if I use array syntax as we had it before react 16, a method or Fragment, I tried all three of them as you can see at the commented out code. The result is the same.

In the original component I also get the <null> element where I have the <ListItem> map, but I was not able to reproduce that in a simple way. It is a quite complex component. But when I understand what I have to do about the null element I will probably be able to fix the problem everywhere.

Raigen avatar May 23 '18 13:05 Raigen

Thanks for reporting. The above is a pretty odd looking example, would it be possible to narrow it down a bit.

From a quick scan it could look like it is the React.Fragment that is rendering as null.

sunesimonsen avatar May 23 '18 17:05 sunesimonsen

What do you get if you narrow it down to:

class Component extends React.Component {
  render() {
    return (
      <div>
        <React.Fragment>
          <h4>Headline 1</h4>
          Text
        </React.Fragment>
      </div>
    )
  }
}

it('should render', function() {
  expect(
    <BadComponent />,
    'to deeply render as',
    <div>
      <h4>Headline 1</h4>
      Text
    </div>
  )
})

sunesimonsen avatar May 23 '18 17:05 sunesimonsen

Hi, thanks for the fast response. The example looks that odd because I tried to mimic our quiet complex component. Since I did not knew what causes the issue I tried to reproduce what we do without doing a lot of things in between.

I tried out your snippet, that one works just fine. So I tried out some other things and think I now know why the <null> element is included. Whenever there is an array or Fragment AND another element on the same layer the result of the array and the content of the fragment are wrapped inside a <null> element. But when I have a look into the React dev-tools in Chrome I can not find those elements. Maybe this is an issue comming from TestUtils?

<div>
  <React.Fragment>
    <h4>Headline 1</h4>
    Text
  </React.Fragment>
</div>

results in

<Component>
  <div>
    <div>
      <h4>Headline 1</h4>
      Text
    </div>
  </div>
</Component>

But

<div>
  <React.Fragment>
    <h4>Headline 1</h4>
    Text
  </React.Fragment>
  <h4>Headline 2</h4>
  Text
</div>

results in

<Component>
  <div>
    <div>
      <null>
        <h4>Headline 1</h4>
        Text
      </null>
      <h4>Headline 2</h4>
      Text
    </div>
  </div>
</Component>

That is also the reason I could not reproduce it for my lists. I forgot a part where we add a fixed element to the end of every list.

const renderList = () => (
  <List>
    {() => (
      <ol>
        {['One', 'Two'].map(t => (
          <ListItem key={t}>
            <li>{t}</li>
          </ListItem>
        ))}
        <ListItem key={100}>
          <li>Fixed list item</li>
        </ListItem>
      </ol>
    )}
  </List>
)

will result in a

<List>
  <ol>
    <null>
      <ListItem>
        <li>One</li>
      </ListItem>
      <ListItem>
        <li>Two</li>
      </ListItem>
    </null>
    <ListItem>
      <li>Fixed list item</li>
    </ListItem>
  </ol>
</List>

We could add the fixed list item to the array to fix the issue there. But then we still have the conditional h4 with a list. I could probably workaround this by adding the condition to both, the h4 and the renderList call. But avoinding Fragment should not be the solution.

Raigen avatar May 24 '18 09:05 Raigen

Thanks that is a much better reproduction example. I'll have to pass it on to @bruderstein for a solution.

sunesimonsen avatar May 25 '18 06:05 sunesimonsen

So are there any news on this topic?

I am currently fixing this issue by doing concat with a lot of components that are on the same DOM level as an array of elements. Almost all projects have a pager element for example with up to 4 additional elements around them for back and forward navigation, and sometimes spacer elements.

Raigen avatar May 30 '18 12:05 Raigen

Thanks for the investigation and repro! I can reproduce, and am investigating the fix.

bruderstein avatar May 31 '18 06:05 bruderstein

@Raigen just a question on this - what would you prefer/expect here: That the React.Fragment was in the assertion, or that the React.Fragment was flattened, as react itself does. I'm leaning strongly towards flattening the React.Fragment, as otherwise you're testing implementation.

WDYT?

bruderstein avatar May 31 '18 07:05 bruderstein

@bruderstein I would also expect it to flatten. Also old tests do not include any wrapping components, not for arrays and not for React.Fragment, so flattening has a higher compatability with existing tests. And developers can see existing tests succeed when moving implementation to React.Fragment without adjusting tests.

Raigen avatar May 31 '18 13:05 Raigen

Did anything happen with this? Some of my tests are having issues with this too.

Lugribossk avatar Sep 26 '18 14:09 Lugribossk