Skip to content

Commit 38621bc

Browse files
sebmarkbagesebmarkbage
committed
Warn for Iterator of all types but allow Generator Components
1 parent 7909d8e commit 38621bc

5 files changed

Lines changed: 245 additions & 70 deletions

File tree

‎packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7913,4 +7913,85 @@ describe('ReactDOMFizzServer', () => {
79137913
]);
79147914
expect(postpones).toEqual([]);
79157915
});
7916+
7917+
it('should NOT warn for using generator functions as components', async () => {
7918+
function* Foo() {
7919+
yield <h1 key="1">Hello</h1>;
7920+
yield <h1 key="2">World</h1>;
7921+
}
7922+
7923+
await act(() => {
7924+
const {pipe} = renderToPipeableStream(<Foo />);
7925+
pipe(writable);
7926+
});
7927+
7928+
expect(document.body.textContent).toBe('HelloWorld');
7929+
});
7930+
7931+
it('should warn for using generators as children props', async () => {
7932+
function* getChildren() {
7933+
yield <h1 key="1">Hello</h1>;
7934+
yield <h1 key="2">World</h1>;
7935+
}
7936+
7937+
function Foo() {
7938+
const children = getChildren();
7939+
return <div>{children}</div>;
7940+
}
7941+
7942+
await expect(async () => {
7943+
await act(() => {
7944+
const {pipe} = renderToPipeableStream(<Foo />);
7945+
pipe(writable);
7946+
});
7947+
}).toErrorDev(
7948+
'Using Iterators as children is unsupported and will likely yield ' +
7949+
'unexpected results because enumerating a generator mutates it. ' +
7950+
'You may convert it to an array with `Array.from()` or the ' +
7951+
'`[...spread]` operator before rendering. You can also use an ' +
7952+
'Iterable that can iterate multiple times over the same items.\n' +
7953+
' in div (at **)\n' +
7954+
' in Foo (at **)',
7955+
);
7956+
7957+
expect(document.body.textContent).toBe('HelloWorld');
7958+
});
7959+
7960+
it('should warn for using other types of iterators as children', async () => {
7961+
function Foo() {
7962+
let i = 0;
7963+
const iterator = {
7964+
[Symbol.iterator]() {
7965+
return iterator;
7966+
},
7967+
next() {
7968+
switch (i++) {
7969+
case 0:
7970+
return {done: false, value: <h1 key="1">Hello</h1>};
7971+
case 1:
7972+
return {done: false, value: <h1 key="2">World</h1>};
7973+
default:
7974+
return {done: true, value: undefined};
7975+
}
7976+
},
7977+
};
7978+
return iterator;
7979+
}
7980+
7981+
await expect(async () => {
7982+
await act(() => {
7983+
const {pipe} = renderToPipeableStream(<Foo />);
7984+
pipe(writable);
7985+
});
7986+
}).toErrorDev(
7987+
'Using Iterators as children is unsupported and will likely yield ' +
7988+
'unexpected results because enumerating a generator mutates it. ' +
7989+
'You may convert it to an array with `Array.from()` or the ' +
7990+
'`[...spread]` operator before rendering. You can also use an ' +
7991+
'Iterable that can iterate multiple times over the same items.\n' +
7992+
' in Foo (at **)',
7993+
);
7994+
7995+
expect(document.body.textContent).toBe('HelloWorld');
7996+
});
79167997
});

‎packages/react-dom/src/__tests__/ReactMultiChild-test.js‎

Lines changed: 73 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -328,26 +328,94 @@ describe('ReactMultiChild', () => {
328328
);
329329
});
330330

331-
it('should warn for using generators as children', async () => {
331+
it('should NOT warn for using generator functions as components', async () => {
332332
function* Foo() {
333333
yield <h1 key="1">Hello</h1>;
334334
yield <h1 key="2">World</h1>;
335335
}
336336

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+
337402
const container = document.createElement('div');
338403
const root = ReactDOMClient.createRoot(container);
339404
await expect(async () => {
340405
await act(async () => {
341406
root.render(<Foo />);
342407
});
343408
}).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' +
348414
' in Foo (at **)',
349415
);
350416

417+
expect(container.textContent).toBe('HelloWorld');
418+
351419
// Test de-duplication
352420
await act(async () => {
353421
root.render(<Foo />);

‎packages/react-reconciler/src/ReactChildFiber.js‎

Lines changed: 47 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,13 @@ import {
3333
REACT_LAZY_TYPE,
3434
REACT_CONTEXT_TYPE,
3535
} 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';
3743
import isArray from 'shared/isArray';
3844
import {enableRefAsProp} from 'shared/ReactFeatureFlags';
3945

@@ -1114,52 +1120,46 @@ function createChildReconciler(
11141120
);
11151121
}
11161122

1123+
const newChildren = iteratorFn.call(newChildrenIterable);
1124+
11171125
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;
11331150
}
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
11391153
if (!didWarnAboutMaps) {
11401154
console.error(
11411155
'Using Maps as children is not supported. ' +
11421156
'Use an array of keyed ReactElements instead.',
11431157
);
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;
11571159
}
11581160
}
11591161
}
11601162

1161-
const newChildren = iteratorFn.call(newChildrenIterable);
1162-
11631163
if (newChildren == null) {
11641164
throw new Error('An iterable object provided no iterator.');
11651165
}
@@ -1172,12 +1172,17 @@ function createChildReconciler(
11721172
let newIdx = 0;
11731173
let nextOldFiber = null;
11741174

1175+
let knownKeys: Set<string> | null = null;
1176+
11751177
let step = newChildren.next();
11761178
for (
11771179
;
11781180
oldFiber !== null && !step.done;
11791181
newIdx++, step = newChildren.next()
11801182
) {
1183+
if (__DEV__) {
1184+
knownKeys = warnOnInvalidKey(step.value, knownKeys, returnFiber);
1185+
}
11811186
if (oldFiber.index > newIdx) {
11821187
nextOldFiber = oldFiber;
11831188
oldFiber = null;
@@ -1237,6 +1242,9 @@ function createChildReconciler(
12371242
// If we don't have any more existing children we can choose a fast path
12381243
// since the rest will all be insertions.
12391244
for (; !step.done; newIdx++, step = newChildren.next()) {
1245+
if (__DEV__) {
1246+
knownKeys = warnOnInvalidKey(step.value, knownKeys, returnFiber);
1247+
}
12401248
const newFiber = createChild(returnFiber, step.value, lanes, debugInfo);
12411249
if (newFiber === null) {
12421250
continue;
@@ -1262,6 +1270,9 @@ function createChildReconciler(
12621270

12631271
// Keep scanning and use the map to restore deleted items as moves.
12641272
for (; !step.done; newIdx++, step = newChildren.next()) {
1273+
if (__DEV__) {
1274+
knownKeys = warnOnInvalidKey(step.value, knownKeys, returnFiber);
1275+
}
12651276
const newFiber = updateFromMap(
12661277
existingChildren,
12671278
returnFiber,

0 commit comments

Comments
 (0)