Skip to content

Commit

Permalink
[cherry-pick] treat empty string as null (facebook#22807)
Browse files Browse the repository at this point in the history 
This one affects the output of some Jest snapshot tests, so I'm going
to land it before the other changes.
  • Loading branch information
@salazarm @acdlite
salazarm authored and acdlite committed Jan 19, 2022
1 parent 4b28ec0 commit cd1f860
Show file tree
Hide file tree
Showing 6 changed files with 102 additions and 41 deletions.
34 changes: 6 additions & 28 deletions packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,17 +87,8 @@ describe('ReactDOMServerIntegration', () => {
{''}
</div>,
);
if (render === serverRender || render === streamRender) {
// For plain server markup result we should have no text nodes if
// they're all empty.
expect(e.childNodes.length).toBe(0);
expect(e.textContent).toBe('');
} else {
expect(e.childNodes.length).toBe(3);
expectTextNode(e.childNodes[0], '');
expectTextNode(e.childNodes[1], '');
expectTextNode(e.childNodes[2], '');
}
expect(e.childNodes.length).toBe(0);
expect(e.textContent).toBe('');
});

itRenders('a div with multiple whitespace children', async render => {
Expand Down Expand Up @@ -162,27 +153,14 @@ describe('ReactDOMServerIntegration', () => {

itRenders('a leading blank child with a text sibling', async render => {
const e = await render(<div>{''}foo</div>);
if (render === serverRender || render === streamRender) {
expect(e.childNodes.length).toBe(1);
expectTextNode(e.childNodes[0], 'foo');
} else {
expect(e.childNodes.length).toBe(2);
expectTextNode(e.childNodes[0], '');
expectTextNode(e.childNodes[1], 'foo');
}
expect(e.childNodes.length).toBe(1);
expectTextNode(e.childNodes[0], 'foo');
});

itRenders('a trailing blank child with a text sibling', async render => {
const e = await render(<div>foo{''}</div>);
// with Fiber, there are just two text nodes.
if (render === serverRender || render === streamRender) {
expect(e.childNodes.length).toBe(1);
expectTextNode(e.childNodes[0], 'foo');
} else {
expect(e.childNodes.length).toBe(2);
expectTextNode(e.childNodes[0], 'foo');
expectTextNode(e.childNodes[1], '');
}
expect(e.childNodes.length).toBe(1);
expectTextNode(e.childNodes[0], 'foo');
});

itRenders('an element with two text children', async render => {
Expand Down
58 changes: 57 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

'use strict';

let React;
let React = require('react');
let ReactDOM;
let ReactDOMServer;
let Scheduler;
Expand Down Expand Up @@ -70,6 +70,17 @@ function dispatchMouseEvent(to, from) {
}
}

class TestAppClass extends React.Component {
render() {
return (
<div>
<>{''}</>
<>{'Hello'}</>
</div>
);
}
}

describe('ReactDOMServerPartialHydration', () => {
beforeEach(() => {
jest.resetModuleRegistry();
Expand Down Expand Up @@ -2738,4 +2749,49 @@ describe('ReactDOMServerPartialHydration', () => {
expect(ref.current).toBe(span);
expect(ref.current.innerHTML).toBe('Hidden child');
});

function itHydratesWithoutMismatch(msg, App) {
it('hydrates without mismatch ' + msg, () => {
const container = document.createElement('div');
document.body.appendChild(container);
const finalHTML = ReactDOMServer.renderToString(<App />);
container.innerHTML = finalHTML;

ReactDOM.hydrateRoot(container, <App />);
Scheduler.unstable_flushAll();
});
}

itHydratesWithoutMismatch('an empty string with neighbors', function App() {
return (
<div>
<div id="test">Test</div>
{'' && <div>Test</div>}
{'Test'}
</div>
);
});

itHydratesWithoutMismatch('an empty string', function App() {
return '';
});
itHydratesWithoutMismatch(
'an empty string simple in fragment',
function App() {
return (
<>
{''}
{'sup'}
</>
);
},
);
itHydratesWithoutMismatch(
'an empty string simple in suspense',
function App() {
return <Suspense>{'' && false}</Suspense>;
},
);

itHydratesWithoutMismatch('an empty string in class component', TestAppClass);
});
7 changes: 5 additions & 2 deletions packages/react-dom/src/__tests__/ReactMultiChildText-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ const expectChildren = function(container, children) {
const child = children[i];

if (typeof child === 'string') {
if (child === '') {
continue;
}
textNode = outerNode.childNodes[mountIndex];
expect(textNode.nodeType).toBe(3);
expect(textNode.data).toBe(child);
Expand Down Expand Up @@ -83,7 +86,7 @@ describe('ReactMultiChildText', () => {
true, [],
0, '0',
1.2, '1.2',
'', '',
'', [],
'foo', 'foo',

[], [],
Expand All @@ -93,7 +96,7 @@ describe('ReactMultiChildText', () => {
[true], [],
[0], ['0'],
[1.2], ['1.2'],
[''], [''],
[''], [],
['foo'], ['foo'],
[<div />], [<div />],

Expand Down
20 changes: 16 additions & 4 deletions packages/react-reconciler/src/ReactChildFiber.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,10 @@ function ChildReconciler(shouldTrackSideEffects) {
newChild: any,
lanes: Lanes,
): Fiber | null {
if (typeof newChild === 'string' || typeof newChild === 'number') {
if (
(typeof newChild === 'string' && newChild !== '') ||
typeof newChild === 'number'
) {
// Text nodes don't have keys. If the previous node is implicitly keyed
// we can continue to replace it without aborting even if it is not a text
// node.
Expand Down Expand Up @@ -564,7 +567,10 @@ function ChildReconciler(shouldTrackSideEffects) {

const key = oldFiber !== null ? oldFiber.key : null;

if (typeof newChild === 'string' || typeof newChild === 'number') {
if (
(typeof newChild === 'string' && newChild !== '') ||
typeof newChild === 'number'
) {
// Text nodes don't have keys. If the previous node is implicitly keyed
// we can continue to replace it without aborting even if it is not a text
// node.
Expand Down Expand Up @@ -626,7 +632,10 @@ function ChildReconciler(shouldTrackSideEffects) {
newChild: any,
lanes: Lanes,
): Fiber | null {
if (typeof newChild === 'string' || typeof newChild === 'number') {
if (
(typeof newChild === 'string' && newChild !== '') ||
typeof newChild === 'number'
) {
// Text nodes don't have keys, so we neither have to check the old nor
// new node for the key. If both are text nodes, they match.
const matchedFiber = existingChildren.get(newIdx) || null;
Expand Down Expand Up @@ -1299,7 +1308,10 @@ function ChildReconciler(shouldTrackSideEffects) {
throwOnInvalidObjectType(returnFiber, newChild);
}

if (typeof newChild === 'string' || typeof newChild === 'number') {
if (
(typeof newChild === 'string' && newChild !== '') ||
typeof newChild === 'number'
) {
return placeSingleChild(
reconcileSingleTextNode(
returnFiber,
Expand Down
20 changes: 16 additions & 4 deletions packages/react-reconciler/src/ReactChildFiber.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,10 @@ function ChildReconciler(shouldTrackSideEffects) {
newChild: any,
lanes: Lanes,
): Fiber | null {
if (typeof newChild === 'string' || typeof newChild === 'number') {
if (
(typeof newChild === 'string' && newChild !== '') ||
typeof newChild === 'number'
) {
// Text nodes don't have keys. If the previous node is implicitly keyed
// we can continue to replace it without aborting even if it is not a text
// node.
Expand Down Expand Up @@ -564,7 +567,10 @@ function ChildReconciler(shouldTrackSideEffects) {

const key = oldFiber !== null ? oldFiber.key : null;

if (typeof newChild === 'string' || typeof newChild === 'number') {
if (
(typeof newChild === 'string' && newChild !== '') ||
typeof newChild === 'number'
) {
// Text nodes don't have keys. If the previous node is implicitly keyed
// we can continue to replace it without aborting even if it is not a text
// node.
Expand Down Expand Up @@ -626,7 +632,10 @@ function ChildReconciler(shouldTrackSideEffects) {
newChild: any,
lanes: Lanes,
): Fiber | null {
if (typeof newChild === 'string' || typeof newChild === 'number') {
if (
(typeof newChild === 'string' && newChild !== '') ||
typeof newChild === 'number'
) {
// Text nodes don't have keys, so we neither have to check the old nor
// new node for the key. If both are text nodes, they match.
const matchedFiber = existingChildren.get(newIdx) || null;
Expand Down Expand Up @@ -1299,7 +1308,10 @@ function ChildReconciler(shouldTrackSideEffects) {
throwOnInvalidObjectType(returnFiber, newChild);
}

if (typeof newChild === 'string' || typeof newChild === 'number') {
if (
(typeof newChild === 'string' && newChild !== '') ||
typeof newChild === 'number'
) {
return placeSingleChild(
reconcileSingleTextNode(
returnFiber,
Expand Down
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -673,7 +673,7 @@ describe('ReactIncrementalUpdates', () => {
root.render(<App />);
});
expect(Scheduler).toHaveYielded(['Committed: ']);
expect(root).toMatchRenderedOutput('');
expect(root).toMatchRenderedOutput(null);

await act(async () => {
if (gate(flags => flags.enableSyncDefaultUpdates)) {
Expand Down Expand Up @@ -734,7 +734,7 @@ describe('ReactIncrementalUpdates', () => {
root.render(<App />);
});
expect(Scheduler).toHaveYielded([]);
expect(root).toMatchRenderedOutput('');
expect(root).toMatchRenderedOutput(null);

await act(async () => {
if (gate(flags => flags.enableSyncDefaultUpdates)) {
Expand Down

0 comments on commit cd1f860

Please sign in to comment.