Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

With react 16 <null> elements are breaking expectation #52

Open
Raigen opened this issue May 23, 2018 · 9 comments
Open

With react 16 <null> elements are breaking expectation #52

Raigen opened this issue May 23, 2018 · 9 comments

Comments

@Raigen
Copy link

Raigen commented May 23, 2018

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.

@sunesimonsen
Copy link
Contributor

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
Copy link
Contributor

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>
  )
})

@Raigen
Copy link
Author

Raigen commented May 24, 2018

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.

@sunesimonsen
Copy link
Contributor

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

@Raigen
Copy link
Author

Raigen commented May 30, 2018

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.

@bruderstein
Copy link
Owner

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

@bruderstein
Copy link
Owner

@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?

@Raigen
Copy link
Author

Raigen commented May 31, 2018

@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.

@Lugribossk
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants