Warn about rendering Generators (#13312)
* Warn about rendering Generators * Fix Flow * Add an explicit test for iterable * Moar test coverage
Dan Abramov committed
Aug 2, 2018 at 20:04 UTC
1609cf3432659a2e793a24ca0f786089257f2a05
3 files changed
+147
-6
packages/react-dom/src/__tests__/ReactMultiChild-test.js
+61
@@ -294,6 +294,67 @@ describe('ReactMultiChild', () => {
294
);
295
});
296
297
+ it('should warn for using generators as children', () => {
298
+ function* Foo() {
299
+ yield <h1 key="1">Hello</h1>;
300
+ yield <h1 key="2">World</h1>;
301
+ }
302
+
303
+ const div = document.createElement('div');
304
+ expect(() => {
305
+ ReactDOM.render(<Foo />, div);
306
+ }).toWarnDev(
307
+ 'Using Generators as children is unsupported and will likely yield ' +
308
+ 'unexpected results because enumerating a generator mutates it. You may ' +
309
+ 'convert it to an array with `Array.from()` or the `[...spread]` operator ' +
310
+ 'before rendering. Keep in mind you might need to polyfill these features for older browsers.\n' +
311
+ ' in Foo (at **)',
312
+ );
313
+
314
+ // Test de-duplication
315
+ ReactDOM.render(<Foo />, div);
316
+ });
317
+
318
+ it('should not warn for using generators in legacy iterables', () => {
319
+ const fooIterable = {
320
+ '@@iterator': function*() {
321
+ yield <h1 key="1">Hello</h1>;
322
+ yield <h1 key="2">World</h1>;
323
+ },
324
+ };
325
+
326
+ function Foo() {
327
+ return fooIterable;
328
+ }
329
+
330
+ const div = document.createElement('div');
331
+ ReactDOM.render(<Foo />, div);
332
+ expect(div.textContent).toBe('HelloWorld');
333
+
334
+ ReactDOM.render(<Foo />, div);
335
+ expect(div.textContent).toBe('HelloWorld');
336
+ });
337
+
338
+ it('should not warn for using generators in modern iterables', () => {
339
+ const fooIterable = {
340
+ [Symbol.iterator]: function*() {
341
+ yield <h1 key="1">Hello</h1>;
342
+ yield <h1 key="2">World</h1>;
343
+ },
344
+ };
345
+
346
+ function Foo() {
347
+ return fooIterable;
348
+ }
349
+
350
+ const div = document.createElement('div');
351
+ ReactDOM.render(<Foo />, div);
352
+ expect(div.textContent).toBe('HelloWorld');
353
+
354
+ ReactDOM.render(<Foo />, div);
355
+ expect(div.textContent).toBe('HelloWorld');
356
+ });
357
+
358
it('should reorder bailed-out children', () => {
359
class LetterInner extends React.Component {
360
render() {
packages/react-dom/src/__tests__/ReactMultiChildReconcile-test.js
+66
-6
@@ -248,7 +248,7 @@ function prepareChildrenArray(childrenArray) {
248
return childrenArray;
249
}
250
251
-function prepareChildrenIterable(childrenArray) {
251
+function prepareChildrenLegacyIterable(childrenArray) {
252
return {
253
'@@iterator': function*() {
254
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
@@ -259,9 +259,27 @@ function prepareChildrenIterable(childrenArray) {
259
};
260
}
261
262
+function prepareChildrenModernIterable(childrenArray) {
263
+ return {
264
+ [Symbol.iterator]: function*() {
265
+ // eslint-disable-next-line no-for-of-loops/no-for-of-loops
266
+ for (const child of childrenArray) {
267
+ yield child;
268
+ }
269
+ },
270
+ };
271
+}
272
+
273
function testPropsSequence(sequence) {
274
testPropsSequenceWithPreparedChildren(sequence, prepareChildrenArray);
264
- testPropsSequenceWithPreparedChildren(sequence, prepareChildrenIterable);
275
+ testPropsSequenceWithPreparedChildren(
276
+ sequence,
277
+ prepareChildrenLegacyIterable,
278
+ );
279
+ testPropsSequenceWithPreparedChildren(
280
+ sequence,
281
+ prepareChildrenModernIterable,
282
+ );
283
}
284
285
describe('ReactMultiChildReconcile', () => {
@@ -311,7 +329,49 @@ describe('ReactMultiChildReconcile', () => {
329
);
330
});
331
314
- it('should reset internal state if removed then readded in an iterable', () => {
332
+ it('should reset internal state if removed then readded in a legacy iterable', () => {
333
+ // Test basics.
334
+ const props = {
335
+ usernameToStatus: {
336
+ jcw: 'jcwStatus',
337
+ },
338
+ };
339
+
340
+ const container = document.createElement('div');
341
+ const parentInstance = ReactDOM.render(
342
+ <FriendsStatusDisplay
343
+ {...props}
344
+ prepareChildren={prepareChildrenLegacyIterable}
345
+ />,
346
+ container,
347
+ );
348
+ let statusDisplays = parentInstance.getStatusDisplays();
349
+ const startingInternalState = statusDisplays.jcw.getInternalState();
350
+
351
+ // Now remove the child.
352
+ ReactDOM.render(
353
+ <FriendsStatusDisplay prepareChildren={prepareChildrenLegacyIterable} />,
354
+ container,
355
+ );
356
+ statusDisplays = parentInstance.getStatusDisplays();
357
+ expect(statusDisplays.jcw).toBeFalsy();
358
+
359
+ // Now reset the props that cause there to be a child
360
+ ReactDOM.render(
361
+ <FriendsStatusDisplay
362
+ {...props}
363
+ prepareChildren={prepareChildrenLegacyIterable}
364
+ />,
365
+ container,
366
+ );
367
+ statusDisplays = parentInstance.getStatusDisplays();
368
+ expect(statusDisplays.jcw).toBeTruthy();
369
+ expect(statusDisplays.jcw.getInternalState()).not.toBe(
370
+ startingInternalState,
371
+ );
372
+ });
373
+
374
+ it('should reset internal state if removed then readded in a modern iterable', () => {
375
// Test basics.
376
const props = {
377
usernameToStatus: {
@@ -323,7 +383,7 @@ describe('ReactMultiChildReconcile', () => {
383
const parentInstance = ReactDOM.render(
384
<FriendsStatusDisplay
385
{...props}
326
- prepareChildren={prepareChildrenIterable}
386
+ prepareChildren={prepareChildrenModernIterable}
387
/>,
388
container,
389
);
@@ -332,7 +392,7 @@ describe('ReactMultiChildReconcile', () => {
392
393
// Now remove the child.
394
ReactDOM.render(
335
- <FriendsStatusDisplay prepareChildren={prepareChildrenIterable} />,
395
+ <FriendsStatusDisplay prepareChildren={prepareChildrenModernIterable} />,
396
container,
397
);
398
statusDisplays = parentInstance.getStatusDisplays();
@@ -342,7 +402,7 @@ describe('ReactMultiChildReconcile', () => {
402
ReactDOM.render(
403
<FriendsStatusDisplay
404
{...props}
345
- prepareChildren={prepareChildrenIterable}
405
+ prepareChildren={prepareChildrenModernIterable}
406
/>,
407
container,
408
);
packages/react-reconciler/src/ReactChildFiber.js
+20
@@ -46,6 +46,7 @@ import {
46
import {StrictMode} from './ReactTypeOfMode';
47
48
let didWarnAboutMaps;
49
+let didWarnAboutGenerators;
50
let didWarnAboutStringRefInStrictMode;
51
let ownerHasKeyUseWarning;
52
let ownerHasFunctionTypeWarning;
@@ -53,6 +54,7 @@ let warnForMissingKey = (child: mixed) => {};
54
55
if (__DEV__) {
56
didWarnAboutMaps = false;
57
+ didWarnAboutGenerators = false;
58
didWarnAboutStringRefInStrictMode = {};
59
60
/**
@@ -903,6 +905,24 @@ function ChildReconciler(shouldTrackSideEffects) {
905
);
906
907
if (__DEV__) {
908
+ // We don't support rendering Generators because it's a mutation.
909
+ // See https://github.com/facebook/react/issues/12995
910
+ if (
911
+ typeof Symbol === 'function' &&
912
+ // $FlowFixMe Flow doesn't know about toStringTag
913
+ newChildrenIterable[Symbol.toStringTag] === 'Generator'
914
+ ) {
915
+ warning(
916
+ didWarnAboutGenerators,
917
+ 'Using Generators as children is unsupported and will likely yield ' +
918
+ 'unexpected results because enumerating a generator mutates it. ' +
919
+ 'You may convert it to an array with `Array.from()` or the ' +
920
+ '`[...spread]` operator before rendering. Keep in mind ' +
921
+ 'you might need to polyfill these features for older browsers.',
922
+ );
923
+ didWarnAboutGenerators = true;
924
+ }
925
+
926
// Warn about using Maps as children
927
if ((newChildrenIterable: any).entries === iteratorFn) {
928
warning(