Remove most comments from HTML generation output (#10029)
Simplifies markup generation by only inserting a simple comments between consecutive text nodes. I also skip past comments and other nodes while hydrating. This leaves them in place instead of being removed by the hydration mechanism. This is more efficient but will also be needed by hydration validator. There's a special case for empty strings. We probably shouldn't have nodes for those at all. For now I special case it by assuming there won't be one so if we need one, we'll insert an empty text node. I also dropped the ID from the react ID.
Sebastian Markbåge committed
Jun 27, 2017 at 10:43 UTC
e955008b9bbee93fcaf423d4afaf4d22023e2c3f
6 files changed
+107
-123
src/renderers/dom/fiber/ReactDOMFiberEntry.js
+9
-6
@@ -118,7 +118,7 @@ function shouldReuseContent(container) {
118
const rootElement = getReactRootElementInContainer(container);
119
return !!(rootElement &&
120
rootElement.nodeType === ELEMENT_NODE &&
121
- rootElement.getAttribute(ID_ATTRIBUTE_NAME));
121
+ rootElement.hasAttribute(ID_ATTRIBUTE_NAME));
122
}
123
124
function shouldAutoFocusHostComponent(type: string, props: Props): boolean {
@@ -392,7 +392,14 @@ var DOMRenderer = ReactFiberReconciler({
392
return instance.nodeType === 1 && type === instance.nodeName.toLowerCase();
393
},
394
395
- canHydrateTextInstance(instance: Instance | TextInstance): boolean {
395
+ canHydrateTextInstance(
396
+ instance: Instance | TextInstance,
397
+ text: string,
398
+ ): boolean {
399
+ if (text === '') {
400
+ // Empty strings are not parsed by HTML so there won't be a correct match here.
401
+ return false;
402
+ }
403
return instance.nodeType === 3;
404
},
405
@@ -401,11 +408,9 @@ var DOMRenderer = ReactFiberReconciler({
408
): null | Instance | TextInstance {
409
let node = instance.nextSibling;
410
// Skip non-hydratable nodes.
404
- /*
411
while (node && node.nodeType !== 1 && node.nodeType !== 3) {
412
node = node.nextSibling;
413
}
408
- */
414
return (node: any);
415
},
416
@@ -414,11 +419,9 @@ var DOMRenderer = ReactFiberReconciler({
419
): null | Instance | TextInstance {
420
let next = parentInstance.firstChild;
421
// Skip non-hydratable nodes.
417
- /*
422
while (next && next.nodeType !== 1 && next.nodeType !== 3) {
423
next = next.nextSibling;
424
}
421
- */
425
return (next: any);
426
},
427
src/renderers/dom/shared/__tests__/ReactDOMServerIntegration-test.js
+61
-84
@@ -606,8 +606,9 @@ describe('ReactDOMServerIntegration', () => {
606
// with Fiber, there are just three separate text node children,
607
// each of which is blank.
608
if (render === serverRender || render === streamRender) {
609
- // For plain server markup result we expect six comment nodes.
610
- expect(e.childNodes.length).toBe(6);
609
+ // For plain server markup result we should have no text nodes if
610
+ // they're all empty.
611
+ expect(e.childNodes.length).toBe(0);
612
expect(e.textContent).toBe('');
613
} else {
614
expect(e.childNodes.length).toBe(3);
@@ -627,12 +628,18 @@ describe('ReactDOMServerIntegration', () => {
628
itRenders('a div with multiple whitespace children', async render => {
629
const e = await render(<div>{' '}{' '}{' '}</div>);
630
if (ReactDOMFeatureFlags.useFiber) {
630
- // with Fiber, there are just three text nodes.
631
- if (render === serverRender || render === streamRender) {
632
- expect(e.childNodes.length).toBe(9);
633
- expectTextNode(e.childNodes[1], ' ');
631
+ // with Fiber, there are normally just three text nodes
632
+ if (
633
+ render === serverRender ||
634
+ render === clientRenderOnServerString ||
635
+ render === streamRender
636
+ ) {
637
+ // For plain server markup result we have comments between.
638
+ // If we're able to hydrate, they remain.
639
+ expect(e.childNodes.length).toBe(5);
640
+ expectTextNode(e.childNodes[0], ' ');
641
+ expectTextNode(e.childNodes[2], ' ');
642
expectTextNode(e.childNodes[4], ' ');
635
- expectTextNode(e.childNodes[7], ' ');
643
} else {
644
expect(e.childNodes.length).toBe(3);
645
expectTextNode(e.childNodes[0], ' ');
@@ -652,10 +659,7 @@ describe('ReactDOMServerIntegration', () => {
659
itRenders('a div with text sibling to a node', async render => {
660
const e = await render(<div>Text<span>More Text</span></div>);
661
let spanNode;
655
- if (
656
- ReactDOMFeatureFlags.useFiber &&
657
- (render !== serverRender && render !== streamRender)
658
- ) {
662
+ if (ReactDOMFeatureFlags.useFiber) {
663
// with Fiber, there are only two children, the "Text" text node and
664
// the span element.
665
expect(e.childNodes.length).toBe(2);
@@ -666,14 +670,7 @@ describe('ReactDOMServerIntegration', () => {
670
expect(e.childNodes.length).toBe(4);
671
spanNode = e.childNodes[3];
672
}
669
- if (
670
- ReactDOMFeatureFlags.useFiber &&
671
- (render === serverRender || render === streamRender)
672
- ) {
673
- expectTextNode(e.childNodes[1], 'Text');
674
- } else {
675
- expectTextNode(e.childNodes[0], 'Text');
676
- }
673
+ expectTextNode(e.childNodes[0], 'Text');
674
expect(spanNode.tagName).toBe('SPAN');
675
expect(spanNode.childNodes.length).toBe(1);
676
expectNode(spanNode.firstChild, TEXT_NODE_TYPE, 'More Text');
@@ -698,8 +695,8 @@ describe('ReactDOMServerIntegration', () => {
695
if (ReactDOMFeatureFlags.useFiber) {
696
// with Fiber, there are just two text nodes.
697
if (render === serverRender || render === streamRender) {
701
- expect(e.childNodes.length).toBe(5);
702
- expectTextNode(e.childNodes[3], 'foo');
698
+ expect(e.childNodes.length).toBe(1);
699
+ expectTextNode(e.childNodes[0], 'foo');
700
} else {
701
expect(e.childNodes.length).toBe(2);
702
expectTextNode(e.childNodes[0], '');
@@ -720,8 +717,8 @@ describe('ReactDOMServerIntegration', () => {
717
if (ReactDOMFeatureFlags.useFiber) {
718
// with Fiber, there are just two text nodes.
719
if (render === serverRender || render === streamRender) {
723
- expect(e.childNodes.length).toBe(5);
724
- expectTextNode(e.childNodes[1], 'foo');
720
+ expect(e.childNodes.length).toBe(1);
721
+ expectTextNode(e.childNodes[0], 'foo');
722
} else {
723
expect(e.childNodes.length).toBe(2);
724
expectTextNode(e.childNodes[0], 'foo');
@@ -741,10 +738,15 @@ describe('ReactDOMServerIntegration', () => {
738
const e = await render(<div>{'foo'}{'bar'}</div>);
739
if (ReactDOMFeatureFlags.useFiber) {
740
// with Fiber, there are just two text nodes.
744
- if (render === serverRender || render === streamRender) {
745
- expect(e.childNodes.length).toBe(6);
746
- expectTextNode(e.childNodes[1], 'foo');
747
- expectTextNode(e.childNodes[4], 'bar');
741
+ if (
742
+ render === serverRender ||
743
+ render === clientRenderOnServerString ||
744
+ render === streamRender
745
+ ) {
746
+ // In the server render output there's a comment between them.
747
+ expect(e.childNodes.length).toBe(3);
748
+ expectTextNode(e.childNodes[0], 'foo');
749
+ expectTextNode(e.childNodes[2], 'bar');
750
} else {
751
expect(e.childNodes.length).toBe(2);
752
expectTextNode(e.childNodes[0], 'foo');
@@ -776,10 +778,15 @@ describe('ReactDOMServerIntegration', () => {
778
const e = await render(<div>{'foo'}{40}</div>);
779
if (ReactDOMFeatureFlags.useFiber) {
780
// with Fiber, there are just two text nodes.
779
- if (render === serverRender || render === streamRender) {
780
- expect(e.childNodes.length).toBe(6);
781
- expectTextNode(e.childNodes[1], 'foo');
782
- expectTextNode(e.childNodes[4], '40');
781
+ if (
782
+ render === serverRender ||
783
+ render === clientRenderOnServerString ||
784
+ render === streamRender
785
+ ) {
786
+ // In the server markup there's a comment between.
787
+ expect(e.childNodes.length).toBe(3);
788
+ expectTextNode(e.childNodes[0], 'foo');
789
+ expectTextNode(e.childNodes[2], '40');
790
} else {
791
expect(e.childNodes.length).toBe(2);
792
expectTextNode(e.childNodes[0], 'foo');
@@ -816,12 +823,7 @@ describe('ReactDOMServerIntegration', () => {
823
const e = await render(<div><NullComponent /></div>);
824
if (ReactDOMFeatureFlags.useFiber) {
825
// with Fiber, an empty component results in no markup.
819
- if (render === serverRender || render === streamRender) {
820
- expect(e.childNodes.length).toBe(1);
821
- expectEmptyNode(e.firstChild);
822
- } else {
823
- expect(e.childNodes.length).toBe(0);
824
- }
826
+ expect(e.childNodes.length).toBe(0);
827
} else {
828
// with Stack, an empty component results in one react-empty comment
829
// node.
@@ -834,13 +836,8 @@ describe('ReactDOMServerIntegration', () => {
836
const e = await render(<div>{null}foo</div>);
837
if (ReactDOMFeatureFlags.useFiber) {
838
// with Fiber, there is just one text node.
837
- if (render === serverRender || render === streamRender) {
838
- expect(e.childNodes.length).toBe(3);
839
- expectTextNode(e.childNodes[1], 'foo');
840
- } else {
841
- expect(e.childNodes.length).toBe(1);
842
- expectTextNode(e.childNodes[0], 'foo');
843
- }
839
+ expect(e.childNodes.length).toBe(1);
840
+ expectTextNode(e.childNodes[0], 'foo');
841
} else {
842
// with Stack, there's a text node surronded by react-text comment nodes.
843
expect(e.childNodes.length).toBe(3);
@@ -852,13 +849,8 @@ describe('ReactDOMServerIntegration', () => {
849
const e = await render(<div>{false}foo</div>);
850
if (ReactDOMFeatureFlags.useFiber) {
851
// with Fiber, there is just one text node.
855
- if (render === serverRender || render === streamRender) {
856
- expect(e.childNodes.length).toBe(3);
857
- expectTextNode(e.childNodes[1], 'foo');
858
- } else {
859
- expect(e.childNodes.length).toBe(1);
860
- expectTextNode(e.childNodes[0], 'foo');
861
- }
852
+ expect(e.childNodes.length).toBe(1);
853
+ expectTextNode(e.childNodes[0], 'foo');
854
} else {
855
// with Stack, there's a text node surronded by react-text comment nodes.
856
expect(e.childNodes.length).toBe(3);
@@ -870,13 +862,8 @@ describe('ReactDOMServerIntegration', () => {
862
const e = await render(<div>{false}{null}foo{null}{false}</div>);
863
if (ReactDOMFeatureFlags.useFiber) {
864
// with Fiber, there is just one text node.
873
- if (render === serverRender || render === streamRender) {
874
- expect(e.childNodes.length).toBe(3);
875
- expectTextNode(e.childNodes[1], 'foo');
876
- } else {
877
- expect(e.childNodes.length).toBe(1);
878
- expectTextNode(e.childNodes[0], 'foo');
879
- }
865
+ expect(e.childNodes.length).toBe(1);
866
+ expectTextNode(e.childNodes[0], 'foo');
867
} else {
868
// with Stack, there's a text node surronded by react-text comment nodes.
869
expect(e.childNodes.length).toBe(3);
@@ -1075,17 +1062,10 @@ describe('ReactDOMServerIntegration', () => {
1062
if (ReactDOMFeatureFlags.useFiber) {
1063
// with Fiber, there are three children: the child1 element, a
1064
// single space text node, and the child2 element.
1078
- if (render === serverRender || render === streamRender) {
1079
- expect(e.childNodes.length).toBe(5);
1080
- child1 = e.childNodes[0];
1081
- textNode = e.childNodes[2];
1082
- child2 = e.childNodes[4];
1083
- } else {
1084
- expect(e.childNodes.length).toBe(3);
1085
- child1 = e.childNodes[0];
1086
- textNode = e.childNodes[1];
1087
- child2 = e.childNodes[2];
1088
- }
1065
+ expect(e.childNodes.length).toBe(3);
1066
+ child1 = e.childNodes[0];
1067
+ textNode = e.childNodes[1];
1068
+ child2 = e.childNodes[2];
1069
} else {
1070
// with Stack, there are five children: the child1 element, a single
1071
// space surrounded by react-text comments, and the child2 element.
@@ -1111,17 +1091,10 @@ describe('ReactDOMServerIntegration', () => {
1091
if (ReactDOMFeatureFlags.useFiber) {
1092
// with Fiber, there are three children: a one-space text node, the
1093
// child element, and a two-space text node.
1114
- if (render === serverRender || render === streamRender) {
1115
- expect(e.childNodes.length).toBe(7);
1116
- textNode1 = e.childNodes[1];
1117
- child = e.childNodes[3];
1118
- textNode2 = e.childNodes[5];
1119
- } else {
1120
- expect(e.childNodes.length).toBe(3);
1121
- textNode1 = e.childNodes[0];
1122
- child = e.childNodes[1];
1123
- textNode2 = e.childNodes[2];
1124
- }
1094
+ expect(e.childNodes.length).toBe(3);
1095
+ textNode1 = e.childNodes[0];
1096
+ child = e.childNodes[1];
1097
+ textNode2 = e.childNodes[2];
1098
} else {
1099
// with Stack, there are 7 children: a one-space text node surrounded
1100
// by react-text comments, the child element, and a two-space text node
@@ -1153,10 +1126,14 @@ describe('ReactDOMServerIntegration', () => {
1126
);
1127
if (ReactDOMFeatureFlags.useFiber) {
1128
// with Fiber, there are just two text nodes.
1156
- if (render === serverRender || render === streamRender) {
1157
- expect(e.childNodes.length).toBe(6);
1158
- expectTextNode(e.childNodes[1], '<span>Text1"</span>');
1159
- expectTextNode(e.childNodes[4], '<span>Text2"</span>');
1129
+ if (
1130
+ render === serverRender ||
1131
+ render === clientRenderOnServerString ||
1132
+ render === streamRender
1133
+ ) {
1134
+ expect(e.childNodes.length).toBe(3);
1135
+ expectTextNode(e.childNodes[0], '<span>Text1"</span>');
1136
+ expectTextNode(e.childNodes[2], '<span>Text2"</span>');
1137
} else {
1138
expect(e.childNodes.length).toBe(2);
1139
expectTextNode(e.childNodes[0], '<span>Text1"</span>');
src/renderers/dom/shared/__tests__/ReactServerRendering-test.js
+19
-11
@@ -53,7 +53,7 @@ describe('ReactDOMServer', () => {
53
ROOT_ATTRIBUTE_NAME +
54
'="" ' +
55
ID_ATTRIBUTE_NAME +
56
- '="[^"]+" ' +
56
+ '="[^"]*" ' +
57
ReactMarkupChecksum.CHECKSUM_ATTR_NAME +
58
'="[^"]+">hello world</span>',
59
),
@@ -68,7 +68,7 @@ describe('ReactDOMServer', () => {
68
ROOT_ATTRIBUTE_NAME +
69
'="" ' +
70
ID_ATTRIBUTE_NAME +
71
- '="[^"]+" ' +
71
+ '="[^"]*" ' +
72
ReactMarkupChecksum.CHECKSUM_ATTR_NAME +
73
'="[^"]+"/>',
74
),
@@ -83,7 +83,7 @@ describe('ReactDOMServer', () => {
83
ROOT_ATTRIBUTE_NAME +
84
'="" ' +
85
ID_ATTRIBUTE_NAME +
86
- '="[^"]+" ' +
86
+ '="[^"]*" ' +
87
ReactMarkupChecksum.CHECKSUM_ATTR_NAME +
88
'="[^"]+"/>',
89
),
@@ -98,7 +98,11 @@ describe('ReactDOMServer', () => {
98
}
99
100
var response = ReactDOMServer.renderToString(<NullComponent />);
101
- expect(response).toBe('<!-- react-empty: 1 -->');
101
+ if (ReactDOMFeatureFlags.useFiber) {
102
+ expect(response).toBe('');
103
+ } else {
104
+ expect(response).toBe('<!-- react-empty: 1 -->');
105
+ }
106
});
107
108
// TODO: Test that listeners are not registered onto any document/container.
@@ -123,14 +127,16 @@ describe('ReactDOMServer', () => {
127
ROOT_ATTRIBUTE_NAME +
128
'="" ' +
129
ID_ATTRIBUTE_NAME +
126
- '="[^"]+" ' +
130
+ '="[^"]*" ' +
131
ReactMarkupChecksum.CHECKSUM_ATTR_NAME +
132
'="[^"]+">' +
133
'<span ' +
134
ID_ATTRIBUTE_NAME +
131
- '="[^"]+">' +
132
- '<!-- react-text: [0-9]+ -->My name is <!-- /react-text -->' +
133
- '<!-- react-text: [0-9]+ -->child<!-- /react-text -->' +
135
+ '="[^"]*">' +
136
+ (ReactDOMFeatureFlags.useFiber
137
+ ? 'My name is <!-- -->child'
138
+ : '<!-- react-text: [0-9]+ -->My name is <!-- /react-text -->' +
139
+ '<!-- react-text: [0-9]+ -->child<!-- /react-text -->') +
140
'</span>' +
141
'</div>',
142
),
@@ -190,11 +196,13 @@ describe('ReactDOMServer', () => {
196
ROOT_ATTRIBUTE_NAME +
197
'="" ' +
198
ID_ATTRIBUTE_NAME +
193
- '="[^"]+" ' +
199
+ '="[^"]*" ' +
200
ReactMarkupChecksum.CHECKSUM_ATTR_NAME +
201
'="[^"]+">' +
196
- '<!-- react-text: [0-9]+ -->Component name: <!-- /react-text -->' +
197
- '<!-- react-text: [0-9]+ -->TestComponent<!-- /react-text -->' +
202
+ (ReactDOMFeatureFlags.useFiber
203
+ ? 'Component name: <!-- -->TestComponent'
204
+ : '<!-- react-text: [0-9]+ -->Component name: <!-- /react-text -->' +
205
+ '<!-- react-text: [0-9]+ -->TestComponent<!-- /react-text -->') +
206
'</span>',
207
),
208
);
src/renderers/shared/fiber/ReactFiberHydrationContext.js
+3
-2
@@ -108,11 +108,12 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
108
switch (fiber.tag) {
109
case HostComponent: {
110
const type = fiber.type;
111
- const props = fiber.memoizedProps;
111
+ const props = fiber.pendingProps;
112
return canHydrateInstance(nextInstance, type, props);
113
}
114
case HostText: {
115
- return canHydrateTextInstance(nextInstance);
115
+ const text = fiber.pendingProps;
116
+ return canHydrateTextInstance(nextInstance, text);
117
}
118
default:
119
return false;
src/renderers/shared/fiber/ReactFiberReconciler.js
+1
-1
@@ -123,7 +123,7 @@ export type HostConfig<T, P, I, TI, PI, C, CX, PL> = {
123
124
// Optional hydration
125
canHydrateInstance?: (instance: I | TI, type: T, props: P) => boolean,
126
- canHydrateTextInstance?: (instance: I | TI) => boolean,
126
+ canHydrateTextInstance?: (instance: I | TI, text: string) => boolean,
127
getNextHydratableSibling?: (instance: I | TI) => null | I | TI,
128
getFirstHydratableChild?: (parentInstance: I | C) => null | I | TI,
129
hydrateInstance?: (
src/renderers/shared/server/ReactPartialRenderer.js
+14
-19
@@ -181,7 +181,6 @@ function createOpenTagMarkup(
181
props,
182
makeStaticMarkup,
183
isRootElement,
184
- domID,
184
instForDebug,
185
) {
186
var ret = '<' + tagVerbatim;
@@ -230,7 +229,7 @@ function createOpenTagMarkup(
229
if (isRootElement) {
230
ret += ' ' + DOMPropertyOperations.createMarkupForRoot();
231
}
233
- ret += ' ' + DOMPropertyOperations.createMarkupForID(domID);
232
+ ret += ' ' + DOMPropertyOperations.createMarkupForID('');
233
return ret;
234
}
235
@@ -346,9 +345,9 @@ class ReactDOMServerRenderer {
345
footer: '',
346
},
347
];
349
- this.idCounter = 1;
348
this.exhausted = false;
349
this.currentSelectValue = null;
350
+ this.previousWasTextNode = false;
351
this.makeStaticMarkup = makeStaticMarkup;
352
}
353
@@ -366,6 +365,7 @@ class ReactDOMServerRenderer {
365
var frame = this.stack[this.stack.length - 1];
366
if (frame.childIndex >= frame.children.length) {
367
out += frame.footer;
368
+ this.previousWasTextNode = false;
369
this.stack.pop();
370
if (frame.tag === 'select') {
371
this.currentSelectValue = null;
@@ -380,26 +380,22 @@ class ReactDOMServerRenderer {
380
381
render(child, context) {
382
if (typeof child === 'string' || typeof child === 'number') {
383
+ var text = '' + child;
384
+ if (text === '') {
385
+ return '';
386
+ }
387
if (this.makeStaticMarkup) {
384
- return escapeTextContentForBrowser('' + child);
388
+ return escapeTextContentForBrowser(text);
389
}
386
- return (
387
- '<!-- react-text: ' +
388
- this.idCounter++ +
389
- ' -->' +
390
- escapeTextContentForBrowser('' + child) +
391
- '<!-- /react-text -->'
392
- );
390
+ if (this.previousWasTextNode) {
391
+ return '<!-- -->' + escapeTextContentForBrowser(text);
392
+ }
393
+ this.previousWasTextNode = true;
394
+ return escapeTextContentForBrowser(text);
395
} else {
396
({child, context} = resolve(child, context));
397
if (child === null || child === false) {
396
- if (this.makeStaticMarkup) {
397
- // Normally we'd insert a comment node, but since this is a situation
398
- // where React won't take over (static pages), we can simply return
399
- // nothing.
400
- return '';
401
- }
402
- return '<!-- react-empty: ' + this.idCounter++ + ' -->';
398
+ return '';
399
} else {
400
return this.renderDOM(child, context);
401
}
@@ -645,7 +641,6 @@ class ReactDOMServerRenderer {
641
props,
642
this.makeStaticMarkup,
643
this.stack.length === 1,
648
- this.idCounter++,
644
null,
645
);
646
var footer = '';