Do not reset inner html for null children #8520
Conversation
This is required to support certain third party scripts as well as other fiber renderers (eg the new ReactART renderer)
| if (isDirectTextChild) { | ||
| // We special case a direct text child of a host node. This is a common | ||
| // case. We won't handle it as a reified child. We will instead handle | ||
| // this in the host environment that also have access to this prop. That | ||
| // avoids allocating another HostText fiber and traversing it. | ||
| nextChildren = null; | ||
| } else if (prevProps && ( | ||
| shouldSetTextContent(prevProps) || | ||
| prevProps.children === null || |
There was a problem hiding this comment.
So I think the reason I added these additional checks is to support the case where we switch from a direct text child to an empty child (null, undefined, or boolean). So these checks should actually be comparing nextProps.children, not prevProps.children.
Could you add a test for these cases? It's my fault those don't already exist :)
There was a problem hiding this comment.
In that case, shouldSetTextContent(prevProps) would be true right?
There was a problem hiding this comment.
Could you add a test for these cases? It's my fault those don't already exist :)
Yup, sure. I'll add another test.
| expect(container.firstChild.innerHTML).toEqual('bonjour'); | ||
|
|
||
| ReactDOM.render(<div />, container); | ||
| expect(container.firstChild.innerHTML).toEqual('bonjour'); |
There was a problem hiding this comment.
You verified this failed before?
There was a problem hiding this comment.
Aye, this test fails if you revert the change to ReactFiberBeginWork.
This is required to support certain third party scripts as well as other fiber renderers (eg the new ReactART renderer).