From 181125a87963415779b456f9b9768468c5c8b2f0 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 2 Aug 2018 19:25:02 +0100 Subject: [PATCH 1/4] Warn about rendering Generators --- .../src/__tests__/ReactMultiChild-test.js | 21 +++++++++++++++++++ .../react-reconciler/src/ReactChildFiber.js | 19 +++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactMultiChild-test.js b/packages/react-dom/src/__tests__/ReactMultiChild-test.js index 906934cfa66b..d9848e4fa0d1 100644 --- a/packages/react-dom/src/__tests__/ReactMultiChild-test.js +++ b/packages/react-dom/src/__tests__/ReactMultiChild-test.js @@ -294,6 +294,27 @@ describe('ReactMultiChild', () => { ); }); + it('should warn for using generators as children', () => { + function* Foo() { + yield

Hello

; + yield

World

; + } + + const div = document.createElement('div'); + expect(() => { + ReactDOM.render(, div); + }).toWarnDev( + 'Using Generators as children is unsupported and will likely yield ' + + 'unexpected results because enumerating a generator mutates it. You may ' + + 'convert it to an array with `Array.from()` or the `[...spread]` operator ' + + 'before rendering. Keep in mind you might need to polyfill these features for older browsers.\n' + + ' in Foo (at **)', + ); + + // Test de-duplication + ReactDOM.render(, div); + }); + it('should reorder bailed-out children', () => { class LetterInner extends React.Component { render() { diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index 97aaac8c2c8b..800569b1041d 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -46,6 +46,7 @@ import { import {StrictMode} from './ReactTypeOfMode'; let didWarnAboutMaps; +let didWarnAboutGenerators; let didWarnAboutStringRefInStrictMode; let ownerHasKeyUseWarning; let ownerHasFunctionTypeWarning; @@ -53,6 +54,7 @@ let warnForMissingKey = (child: mixed) => {}; if (__DEV__) { didWarnAboutMaps = false; + didWarnAboutGenerators = false; didWarnAboutStringRefInStrictMode = {}; /** @@ -903,6 +905,23 @@ function ChildReconciler(shouldTrackSideEffects) { ); if (__DEV__) { + // We don't support rendering Generators because it's a mutation. + // See https://github.com/facebook/react/issues/12995 + if ( + typeof Symbol === 'function' && + newChildrenIterable[Symbol.toStringTag] === 'Generator' + ) { + warning( + didWarnAboutGenerators, + 'Using Generators as children is unsupported and will likely yield ' + + 'unexpected results because enumerating a generator mutates it. ' + + 'You may convert it to an array with `Array.from()` or the ' + + '`[...spread]` operator before rendering. Keep in mind ' + + 'you might need to polyfill these features for older browsers.', + ); + didWarnAboutGenerators = true; + } + // Warn about using Maps as children if ((newChildrenIterable: any).entries === iteratorFn) { warning( From 00c2f4da5bf0966fbab1b5c4c2ec912a7cecfe45 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 2 Aug 2018 19:33:08 +0100 Subject: [PATCH 2/4] Fix Flow --- packages/react-reconciler/src/ReactChildFiber.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index 800569b1041d..1ebe9863ebc5 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -909,6 +909,7 @@ function ChildReconciler(shouldTrackSideEffects) { // See https://github.com/facebook/react/issues/12995 if ( typeof Symbol === 'function' && + // $FlowFixMe Flow doesn't know about toStringTag newChildrenIterable[Symbol.toStringTag] === 'Generator' ) { warning( From 9448f2e2da18095691edd7e6bd71ca4194cbbadd Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 2 Aug 2018 19:39:25 +0100 Subject: [PATCH 3/4] Add an explicit test for iterable --- .../src/__tests__/ReactMultiChild-test.js | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactMultiChild-test.js b/packages/react-dom/src/__tests__/ReactMultiChild-test.js index d9848e4fa0d1..59b54228ae34 100644 --- a/packages/react-dom/src/__tests__/ReactMultiChild-test.js +++ b/packages/react-dom/src/__tests__/ReactMultiChild-test.js @@ -315,6 +315,26 @@ describe('ReactMultiChild', () => { ReactDOM.render(, div); }); + it('should not warn for using generators in iterables', () => { + const fooIterable = { + '@@iterator': function*() { + yield

Hello

; + yield

World

; + }, + }; + + function Foo() { + return fooIterable; + } + + const div = document.createElement('div'); + ReactDOM.render(, div); + expect(div.textContent).toBe('HelloWorld'); + + ReactDOM.render(, div); + expect(div.textContent).toBe('HelloWorld'); + }); + it('should reorder bailed-out children', () => { class LetterInner extends React.Component { render() { From 5808caa659e16f0cd4ccb5c898334808c2e0143e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 2 Aug 2018 19:45:50 +0100 Subject: [PATCH 4/4] Moar test coverage --- .../src/__tests__/ReactMultiChild-test.js | 22 +++++- .../ReactMultiChildReconcile-test.js | 72 +++++++++++++++++-- 2 files changed, 87 insertions(+), 7 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactMultiChild-test.js b/packages/react-dom/src/__tests__/ReactMultiChild-test.js index 59b54228ae34..e1d4d87ea75e 100644 --- a/packages/react-dom/src/__tests__/ReactMultiChild-test.js +++ b/packages/react-dom/src/__tests__/ReactMultiChild-test.js @@ -315,7 +315,7 @@ describe('ReactMultiChild', () => { ReactDOM.render(, div); }); - it('should not warn for using generators in iterables', () => { + it('should not warn for using generators in legacy iterables', () => { const fooIterable = { '@@iterator': function*() { yield

Hello

; @@ -335,6 +335,26 @@ describe('ReactMultiChild', () => { expect(div.textContent).toBe('HelloWorld'); }); + it('should not warn for using generators in modern iterables', () => { + const fooIterable = { + [Symbol.iterator]: function*() { + yield

Hello

; + yield

World

; + }, + }; + + function Foo() { + return fooIterable; + } + + const div = document.createElement('div'); + ReactDOM.render(, div); + expect(div.textContent).toBe('HelloWorld'); + + ReactDOM.render(, div); + expect(div.textContent).toBe('HelloWorld'); + }); + it('should reorder bailed-out children', () => { class LetterInner extends React.Component { render() { diff --git a/packages/react-dom/src/__tests__/ReactMultiChildReconcile-test.js b/packages/react-dom/src/__tests__/ReactMultiChildReconcile-test.js index 090230be3e7a..f04b7ece7aab 100644 --- a/packages/react-dom/src/__tests__/ReactMultiChildReconcile-test.js +++ b/packages/react-dom/src/__tests__/ReactMultiChildReconcile-test.js @@ -248,7 +248,7 @@ function prepareChildrenArray(childrenArray) { return childrenArray; } -function prepareChildrenIterable(childrenArray) { +function prepareChildrenLegacyIterable(childrenArray) { return { '@@iterator': function*() { // eslint-disable-next-line no-for-of-loops/no-for-of-loops @@ -259,9 +259,27 @@ function prepareChildrenIterable(childrenArray) { }; } +function prepareChildrenModernIterable(childrenArray) { + return { + [Symbol.iterator]: function*() { + // eslint-disable-next-line no-for-of-loops/no-for-of-loops + for (const child of childrenArray) { + yield child; + } + }, + }; +} + function testPropsSequence(sequence) { testPropsSequenceWithPreparedChildren(sequence, prepareChildrenArray); - testPropsSequenceWithPreparedChildren(sequence, prepareChildrenIterable); + testPropsSequenceWithPreparedChildren( + sequence, + prepareChildrenLegacyIterable, + ); + testPropsSequenceWithPreparedChildren( + sequence, + prepareChildrenModernIterable, + ); } describe('ReactMultiChildReconcile', () => { @@ -311,7 +329,49 @@ describe('ReactMultiChildReconcile', () => { ); }); - it('should reset internal state if removed then readded in an iterable', () => { + it('should reset internal state if removed then readded in a legacy iterable', () => { + // Test basics. + const props = { + usernameToStatus: { + jcw: 'jcwStatus', + }, + }; + + const container = document.createElement('div'); + const parentInstance = ReactDOM.render( + , + container, + ); + let statusDisplays = parentInstance.getStatusDisplays(); + const startingInternalState = statusDisplays.jcw.getInternalState(); + + // Now remove the child. + ReactDOM.render( + , + container, + ); + statusDisplays = parentInstance.getStatusDisplays(); + expect(statusDisplays.jcw).toBeFalsy(); + + // Now reset the props that cause there to be a child + ReactDOM.render( + , + container, + ); + statusDisplays = parentInstance.getStatusDisplays(); + expect(statusDisplays.jcw).toBeTruthy(); + expect(statusDisplays.jcw.getInternalState()).not.toBe( + startingInternalState, + ); + }); + + it('should reset internal state if removed then readded in a modern iterable', () => { // Test basics. const props = { usernameToStatus: { @@ -323,7 +383,7 @@ describe('ReactMultiChildReconcile', () => { const parentInstance = ReactDOM.render( , container, ); @@ -332,7 +392,7 @@ describe('ReactMultiChildReconcile', () => { // Now remove the child. ReactDOM.render( - , + , container, ); statusDisplays = parentInstance.getStatusDisplays(); @@ -342,7 +402,7 @@ describe('ReactMultiChildReconcile', () => { ReactDOM.render( , container, );