Warn for Child Iterator of all types but allow Generator Components (#28853)
This doesn't change production behavior. We always render Iterables to our best effort in prod even if they're Iterators. But this does change the DEV warnings which indicates which are valid patterns to use. It's a footgun to use an Iterator as a prop when you pass between components because if an intermediate component rerenders without its parent, React won't be able to iterate it again to reconcile and any mappers won't be able to re-apply. This is actually typically not a problem when passed only to React host components but as a pattern it's a problem for composability. We used to warn only for Generators - i.e. Iterators returned from Generator functions. This adds a warning for Iterators created by other means too (e.g. Flight or the native Iterator utils). The heuristic is to check whether the Iterator is the same as the Iterable because that means it's not possible to get new iterators out of it. This case used to just yield non-sense like empty sets in DEV but not in prod. However, a new realization is that when the Component itself is a Generator Function, it's not actually a problem. That's because the React Element itself works as an Iterable since we can ask for new generators by calling the function again. So this adds a special case to allow the Generator returned from a Generator Function's direct child. The principle is “don’t pass iterators around” but in this case there is no iterator floating around because it’s between React and the JS VM. Also see #28849 for context on AsyncIterables. Related to this, but Hooks should ideally be banned in these for the same reason they're banned in Async Functions.
Sebastian Markbåge committed
Apr 21, 2024 at 12:51 UTC
368202181e772d411b2445930aea1edd9428b09b
5 files changed
+262
-73
packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
+81
@@ -7984,4 +7984,85 @@ describe('ReactDOMFizzServer', () => {
7984
]);
7985
expect(postpones).toEqual([]);
7986
});
7987
+
7988
+ it('should NOT warn for using generator functions as components', async () => {
7989
+ function* Foo() {
7990
+ yield <h1 key="1">Hello</h1>;
7991
+ yield <h1 key="2">World</h1>;
7992
+ }
7993
+
7994
+ await act(() => {
7995
+ const {pipe} = renderToPipeableStream(<Foo />);
7996
+ pipe(writable);
7997
+ });
7998
+
7999
+ expect(document.body.textContent).toBe('HelloWorld');
8000
+ });
8001
+
8002
+ it('should warn for using generators as children props', async () => {
8003
+ function* getChildren() {
8004
+ yield <h1 key="1">Hello</h1>;
8005
+ yield <h1 key="2">World</h1>;
8006
+ }
8007
+
8008
+ function Foo() {
8009
+ const children = getChildren();
8010
+ return <div>{children}</div>;
8011
+ }
8012
+
8013
+ await expect(async () => {
8014
+ await act(() => {
8015
+ const {pipe} = renderToPipeableStream(<Foo />);
8016
+ pipe(writable);
8017
+ });
8018
+ }).toErrorDev(
8019
+ 'Using Iterators as children is unsupported and will likely yield ' +
8020
+ 'unexpected results because enumerating a generator mutates it. ' +
8021
+ 'You may convert it to an array with `Array.from()` or the ' +
8022
+ '`[...spread]` operator before rendering. You can also use an ' +
8023
+ 'Iterable that can iterate multiple times over the same items.\n' +
8024
+ ' in div (at **)\n' +
8025
+ ' in Foo (at **)',
8026
+ );
8027
+
8028
+ expect(document.body.textContent).toBe('HelloWorld');
8029
+ });
8030
+
8031
+ it('should warn for using other types of iterators as children', async () => {
8032
+ function Foo() {
8033
+ let i = 0;
8034
+ const iterator = {
8035
+ [Symbol.iterator]() {
8036
+ return iterator;
8037
+ },
8038
+ next() {
8039
+ switch (i++) {
8040
+ case 0:
8041
+ return {done: false, value: <h1 key="1">Hello</h1>};
8042
+ case 1:
8043
+ return {done: false, value: <h1 key="2">World</h1>};
8044
+ default:
8045
+ return {done: true, value: undefined};
8046
+ }
8047
+ },
8048
+ };
8049
+ return iterator;
8050
+ }
8051
+
8052
+ await expect(async () => {
8053
+ await act(() => {
8054
+ const {pipe} = renderToPipeableStream(<Foo />);
8055
+ pipe(writable);
8056
+ });
8057
+ }).toErrorDev(
8058
+ 'Using Iterators as children is unsupported and will likely yield ' +
8059
+ 'unexpected results because enumerating a generator mutates it. ' +
8060
+ 'You may convert it to an array with `Array.from()` or the ' +
8061
+ '`[...spread]` operator before rendering. You can also use an ' +
8062
+ 'Iterable that can iterate multiple times over the same items.\n' +
8063
+ ' in Foo (at **)',
8064
+ );
8065
+
8066
+ expect(document.body.textContent).toBe('HelloWorld');
8067
+ });
8068
});
packages/react-dom/src/__tests__/ReactMultiChild-test.js
+73
-5
@@ -328,12 +328,77 @@ describe('ReactMultiChild', () => {
328
);
329
});
330
331
- it('should warn for using generators as children', async () => {
331
+ it('should NOT warn for using generator functions as components', async () => {
332
function* Foo() {
333
yield <h1 key="1">Hello</h1>;
334
yield <h1 key="2">World</h1>;
335
}
336
337
+ const container = document.createElement('div');
338
+ const root = ReactDOMClient.createRoot(container);
339
+ await act(async () => {
340
+ root.render(<Foo />);
341
+ });
342
+
343
+ expect(container.textContent).toBe('HelloWorld');
344
+ });
345
+
346
+ it('should warn for using generators as children props', async () => {
347
+ function* getChildren() {
348
+ yield <h1 key="1">Hello</h1>;
349
+ yield <h1 key="2">World</h1>;
350
+ }
351
+
352
+ function Foo() {
353
+ const children = getChildren();
354
+ return <div>{children}</div>;
355
+ }
356
+
357
+ const container = document.createElement('div');
358
+ const root = ReactDOMClient.createRoot(container);
359
+ await expect(async () => {
360
+ await act(async () => {
361
+ root.render(<Foo />);
362
+ });
363
+ }).toErrorDev(
364
+ 'Using Iterators as children is unsupported and will likely yield ' +
365
+ 'unexpected results because enumerating a generator mutates it. ' +
366
+ 'You may convert it to an array with `Array.from()` or the ' +
367
+ '`[...spread]` operator before rendering. You can also use an ' +
368
+ 'Iterable that can iterate multiple times over the same items.\n' +
369
+ ' in div (at **)\n' +
370
+ ' in Foo (at **)',
371
+ );
372
+
373
+ expect(container.textContent).toBe('HelloWorld');
374
+
375
+ // Test de-duplication
376
+ await act(async () => {
377
+ root.render(<Foo />);
378
+ });
379
+ });
380
+
381
+ it('should warn for using other types of iterators as children', async () => {
382
+ function Foo() {
383
+ let i = 0;
384
+ const iterator = {
385
+ [Symbol.iterator]() {
386
+ return iterator;
387
+ },
388
+ next() {
389
+ switch (i++) {
390
+ case 0:
391
+ return {done: false, value: <h1 key="1">Hello</h1>};
392
+ case 1:
393
+ return {done: false, value: <h1 key="2">World</h1>};
394
+ default:
395
+ return {done: true, value: undefined};
396
+ }
397
+ },
398
+ };
399
+ return iterator;
400
+ }
401
+
402
const container = document.createElement('div');
403
const root = ReactDOMClient.createRoot(container);
404
await expect(async () => {
@@ -341,13 +406,16 @@ describe('ReactMultiChild', () => {
406
root.render(<Foo />);
407
});
408
}).toErrorDev(
344
- 'Using Generators as children is unsupported and will likely yield ' +
345
- 'unexpected results because enumerating a generator mutates it. You may ' +
346
- 'convert it to an array with `Array.from()` or the `[...spread]` operator ' +
347
- 'before rendering. Keep in mind you might need to polyfill these features for older browsers.\n' +
409
+ 'Using Iterators as children is unsupported and will likely yield ' +
410
+ 'unexpected results because enumerating a generator mutates it. ' +
411
+ 'You may convert it to an array with `Array.from()` or the ' +
412
+ '`[...spread]` operator before rendering. You can also use an ' +
413
+ 'Iterable that can iterate multiple times over the same items.\n' +
414
' in Foo (at **)',
415
);
416
417
+ expect(container.textContent).toBe('HelloWorld');
418
+
419
// Test de-duplication
420
await act(async () => {
421
root.render(<Foo />);
packages/react-reconciler/src/ReactChildFiber.js
+64
-39
@@ -33,7 +33,13 @@ import {
33
REACT_LAZY_TYPE,
34
REACT_CONTEXT_TYPE,
35
} from 'shared/ReactSymbols';
36
-import {HostRoot, HostText, HostPortal, Fragment} from './ReactWorkTags';
36
+import {
37
+ HostRoot,
38
+ HostText,
39
+ HostPortal,
40
+ Fragment,
41
+ FunctionComponent,
42
+} from './ReactWorkTags';
43
import isArray from 'shared/isArray';
44
import {enableRefAsProp} from 'shared/ReactFeatureFlags';
45
@@ -1114,52 +1120,46 @@ function createChildReconciler(
1120
);
1121
}
1122
1123
+ const newChildren = iteratorFn.call(newChildrenIterable);
1124
+
1125
if (__DEV__) {
1118
- // We don't support rendering Generators because it's a mutation.
1119
- // See https://github.com/facebook/react/issues/12995
1120
- if (
1121
- typeof Symbol === 'function' &&
1122
- // $FlowFixMe[prop-missing] Flow doesn't know about toStringTag
1123
- newChildrenIterable[Symbol.toStringTag] === 'Generator'
1124
- ) {
1125
- if (!didWarnAboutGenerators) {
1126
- console.error(
1127
- 'Using Generators as children is unsupported and will likely yield ' +
1128
- 'unexpected results because enumerating a generator mutates it. ' +
1129
- 'You may convert it to an array with `Array.from()` or the ' +
1130
- '`[...spread]` operator before rendering. Keep in mind ' +
1131
- 'you might need to polyfill these features for older browsers.',
1132
- );
1126
+ if (newChildren === newChildrenIterable) {
1127
+ // We don't support rendering Generators as props because it's a mutation.
1128
+ // See https://github.com/facebook/react/issues/12995
1129
+ // We do support generators if they were created by a GeneratorFunction component
1130
+ // as its direct child since we can recreate those by rerendering the component
1131
+ // as needed.
1132
+ const isGeneratorComponent =
1133
+ returnFiber.tag === FunctionComponent &&
1134
+ // $FlowFixMe[method-unbinding]
1135
+ Object.prototype.toString.call(returnFiber.type) ===
1136
+ '[object GeneratorFunction]' &&
1137
+ // $FlowFixMe[method-unbinding]
1138
+ Object.prototype.toString.call(newChildren) === '[object Generator]';
1139
+ if (!isGeneratorComponent) {
1140
+ if (!didWarnAboutGenerators) {
1141
+ console.error(
1142
+ 'Using Iterators as children is unsupported and will likely yield ' +
1143
+ 'unexpected results because enumerating a generator mutates it. ' +
1144
+ 'You may convert it to an array with `Array.from()` or the ' +
1145
+ '`[...spread]` operator before rendering. You can also use an ' +
1146
+ 'Iterable that can iterate multiple times over the same items.',
1147
+ );
1148
+ }
1149
+ didWarnAboutGenerators = true;
1150
}
1134
- didWarnAboutGenerators = true;
1135
- }
1136
-
1137
- // Warn about using Maps as children
1138
- if ((newChildrenIterable: any).entries === iteratorFn) {
1151
+ } else if ((newChildrenIterable: any).entries === iteratorFn) {
1152
+ // Warn about using Maps as children
1153
if (!didWarnAboutMaps) {
1154
console.error(
1155
'Using Maps as children is not supported. ' +
1156
'Use an array of keyed ReactElements instead.',
1157
);
1144
- }
1145
- didWarnAboutMaps = true;
1146
- }
1147
-
1148
- // First, validate keys.
1149
- // We'll get a different iterator later for the main pass.
1150
- const newChildren = iteratorFn.call(newChildrenIterable);
1151
- if (newChildren) {
1152
- let knownKeys: Set<string> | null = null;
1153
- let step = newChildren.next();
1154
- for (; !step.done; step = newChildren.next()) {
1155
- const child = step.value;
1156
- knownKeys = warnOnInvalidKey(child, knownKeys, returnFiber);
1158
+ didWarnAboutMaps = true;
1159
}
1160
}
1161
}
1162
1161
- const newChildren = iteratorFn.call(newChildrenIterable);
1162
-
1163
if (newChildren == null) {
1164
throw new Error('An iterable object provided no iterator.');
1165
}
@@ -1172,11 +1172,20 @@ function createChildReconciler(
1172
let newIdx = 0;
1173
let nextOldFiber = null;
1174
1175
+ let knownKeys: Set<string> | null = null;
1176
+
1177
let step = newChildren.next();
1178
+ if (__DEV__) {
1179
+ knownKeys = warnOnInvalidKey(step.value, knownKeys, returnFiber);
1180
+ }
1181
for (
1182
;
1183
oldFiber !== null && !step.done;
1179
- newIdx++, step = newChildren.next()
1184
+ newIdx++,
1185
+ step = newChildren.next(),
1186
+ knownKeys = __DEV__
1187
+ ? warnOnInvalidKey(step.value, knownKeys, returnFiber)
1188
+ : null
1189
) {
1190
if (oldFiber.index > newIdx) {
1191
nextOldFiber = oldFiber;
@@ -1236,7 +1245,15 @@ function createChildReconciler(
1245
if (oldFiber === null) {
1246
// If we don't have any more existing children we can choose a fast path
1247
// since the rest will all be insertions.
1239
- for (; !step.done; newIdx++, step = newChildren.next()) {
1248
+ for (
1249
+ ;
1250
+ !step.done;
1251
+ newIdx++,
1252
+ step = newChildren.next(),
1253
+ knownKeys = __DEV__
1254
+ ? warnOnInvalidKey(step.value, knownKeys, returnFiber)
1255
+ : null
1256
+ ) {
1257
const newFiber = createChild(returnFiber, step.value, lanes, debugInfo);
1258
if (newFiber === null) {
1259
continue;
@@ -1261,7 +1278,15 @@ function createChildReconciler(
1278
const existingChildren = mapRemainingChildren(oldFiber);
1279
1280
// Keep scanning and use the map to restore deleted items as moves.
1264
- for (; !step.done; newIdx++, step = newChildren.next()) {
1281
+ for (
1282
+ ;
1283
+ !step.done;
1284
+ newIdx++,
1285
+ step = newChildren.next(),
1286
+ knownKeys = __DEV__
1287
+ ? warnOnInvalidKey(step.value, knownKeys, returnFiber)
1288
+ : null
1289
+ ) {
1290
const newFiber = updateFromMap(
1291
existingChildren,
1292
returnFiber,
packages/react-server/src/ReactFizzServer.js
+38
-25
@@ -2150,36 +2150,49 @@ function replayElement(
2150
// rendered in the prelude and skip it.
2151
}
2152
2153
-// $FlowFixMe[missing-local-annot]
2154
-function validateIterable(iterable, iteratorFn: Function): void {
2153
+function validateIterable(
2154
+ task: Task,
2155
+ iterable: Iterable<any>,
2156
+ childIndex: number,
2157
+ iterator: Iterator<any>,
2158
+ iteratorFn: () => ?Iterator<any>,
2159
+): void {
2160
if (__DEV__) {
2156
- // We don't support rendering Generators because it's a mutation.
2157
- // See https://github.com/facebook/react/issues/12995
2158
- if (
2159
- typeof Symbol === 'function' &&
2160
- iterable[Symbol.toStringTag] === 'Generator'
2161
- ) {
2162
- if (!didWarnAboutGenerators) {
2163
- console.error(
2164
- 'Using Generators as children is unsupported and will likely yield ' +
2165
- 'unexpected results because enumerating a generator mutates it. ' +
2166
- 'You may convert it to an array with `Array.from()` or the ' +
2167
- '`[...spread]` operator before rendering. Keep in mind ' +
2168
- 'you might need to polyfill these features for older browsers.',
2169
- );
2161
+ if (iterator === iterable) {
2162
+ // We don't support rendering Generators as props because it's a mutation.
2163
+ // See https://github.com/facebook/react/issues/12995
2164
+ // We do support generators if they were created by a GeneratorFunction component
2165
+ // as its direct child since we can recreate those by rerendering the component
2166
+ // as needed.
2167
+ const isGeneratorComponent =
2168
+ task.componentStack !== null &&
2169
+ task.componentStack.tag === 1 && // FunctionComponent
2170
+ // $FlowFixMe[method-unbinding]
2171
+ Object.prototype.toString.call(task.componentStack.type) ===
2172
+ '[object GeneratorFunction]' &&
2173
+ // $FlowFixMe[method-unbinding]
2174
+ Object.prototype.toString.call(iterator) === '[object Generator]';
2175
+ if (!isGeneratorComponent) {
2176
+ if (!didWarnAboutGenerators) {
2177
+ console.error(
2178
+ 'Using Iterators as children is unsupported and will likely yield ' +
2179
+ 'unexpected results because enumerating a generator mutates it. ' +
2180
+ 'You may convert it to an array with `Array.from()` or the ' +
2181
+ '`[...spread]` operator before rendering. You can also use an ' +
2182
+ 'Iterable that can iterate multiple times over the same items.',
2183
+ );
2184
+ }
2185
+ didWarnAboutGenerators = true;
2186
}
2171
- didWarnAboutGenerators = true;
2172
- }
2173
-
2174
- // Warn about using Maps as children
2175
- if ((iterable: any).entries === iteratorFn) {
2187
+ } else if ((iterable: any).entries === iteratorFn) {
2188
+ // Warn about using Maps as children
2189
if (!didWarnAboutMaps) {
2190
console.error(
2191
'Using Maps as children is not supported. ' +
2192
'Use an array of keyed ReactElements instead.',
2193
);
2194
+ didWarnAboutMaps = true;
2195
}
2182
- didWarnAboutMaps = true;
2196
}
2197
}
2198
}
@@ -2303,11 +2316,11 @@ function renderNodeDestructive(
2316
2317
const iteratorFn = getIteratorFn(node);
2318
if (iteratorFn) {
2306
- if (__DEV__) {
2307
- validateIterable(node, iteratorFn);
2308
- }
2319
const iterator = iteratorFn.call(node);
2320
if (iterator) {
2321
+ if (__DEV__) {
2322
+ validateIterable(task, node, childIndex, iterator, iteratorFn);
2323
+ }
2324
// We need to know how many total children are in this set, so that we
2325
// can allocate enough id slots to acommodate them. So we must exhaust
2326
// the iterator before we start recursively rendering the children.
packages/react/src/jsx/ReactJSXElement.js
+6
-4
@@ -1028,10 +1028,12 @@ function validateChildKeys(node, parentType) {
1028
// but now we print a separate warning for them later.
1029
if (iteratorFn !== node.entries) {
1030
const iterator = iteratorFn.call(node);
1031
- let step;
1032
- while (!(step = iterator.next()).done) {
1033
- if (isValidElement(step.value)) {
1034
- validateExplicitKey(step.value, parentType);
1031
+ if (iterator !== node) {
1032
+ let step;
1033
+ while (!(step = iterator.next()).done) {
1034
+ if (isValidElement(step.value)) {
1035
+ validateExplicitKey(step.value, parentType);
1036
+ }
1037
}
1038
}
1039
}