Merge pull request #8936 from acdlite/removemapsaschildren

Remove experimental support for maps as children

Author: acdlite

scripts/fiber/tests-failing.txt

@@ -47,8 +47,5 @@ src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js
 src/renderers/shared/shared/__tests__/ReactComponentLifeCycle-test.js
 * should carry through each of the phases of setup
 
-src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js
-* should reorder keyed text nodes
-
 src/renderers/shared/shared/__tests__/refs-test.js
 * Should increase refs with an increase in divs

scripts/fiber/tests-passing-except-dev.txt

@@ -111,8 +111,5 @@ src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js
 * should warn about `setState` in getChildContext
 * should disallow nested render calls
 
-src/renderers/shared/shared/__tests__/ReactMultiChild-test.js
-* should warn for using maps as children with owner info
-
 src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js
 * should warn for childContextTypes on a functional component

scripts/fiber/tests-passing.txt

@@ -124,7 +124,6 @@ src/isomorphic/children/__tests__/ReactChildren-test.js
 * should retain key across two mappings
 * should be called for each child in an iterable without keys
 * should be called for each child in an iterable with keys
-* should use keys from entry iterables
 * should not enumerate enumerable numbers (#4776)
 * should allow extension of native prototypes
 * should pass key to returned component
@@ -1513,6 +1512,7 @@ src/renderers/shared/shared/__tests__/ReactMultiChild-test.js
 * should replace children with different keys
 * should warn for duplicated array keys with component stack info
 * should warn for duplicated iterable keys with component stack info
+* should warn for using maps as children with owner info
 * should reorder bailed-out children
 * prepares new children before unmounting old
 

src/isomorphic/children/__tests__/ReactChildren-test.js

@@ -105,7 +105,7 @@ describe('ReactChildren', () => {
       expect(callback).toHaveBeenCalledWith(one, 1);
       expect(callback).toHaveBeenCalledWith(two, 2);
       expect(callback).toHaveBeenCalledWith(three, 3);
-      expect(callback).toHaveBeenCalledWith(four, 4);      
+      expect(callback).toHaveBeenCalledWith(four, 4);
       callback.calls.reset();
     }
 
@@ -365,68 +365,6 @@ describe('ReactChildren', () => {
     ]);
   });
 
-  it('should use keys from entry iterables', () => {
-    spyOn(console, 'error');
-
-    var threeDivEntryIterable = {
-      '@@iterator': function() {
-        var i = 0;
-        return {
-          next: function() {
-            if (i++ < 3) {
-              return {value: ['#' + i, <div />], done: false};
-            } else {
-              return {value: undefined, done: true};
-            }
-          },
-        };
-      },
-    };
-    threeDivEntryIterable.entries = threeDivEntryIterable['@@iterator'];
-
-    var context = {};
-    var callback =
-      jasmine.createSpy().and.callFake(function(kid) {
-        expect(this).toBe(context);
-        return kid;
-      });
-
-    var instance = (
-      <div>
-        {threeDivEntryIterable}
-      </div>
-    );
-
-    function assertCalls() {
-      expect(callback.calls.count()).toBe(3);
-      // TODO: why
-      expect(callback).toHaveBeenCalledWith(<div />, 0);
-      expect(callback).toHaveBeenCalledWith(<div />, 1);
-      expect(callback).toHaveBeenCalledWith(<div />, 2);
-      
-      callback.calls.reset();
-    }
-
-    React.Children.forEach(instance.props.children, callback, context);
-    assertCalls();
-    expectDev(console.error.calls.count()).toBe(1);
-    expectDev(console.error.calls.argsFor(0)[0]).toContain(
-      'Warning: Using Maps as children is not yet fully supported. It is an ' +
-      'experimental feature that might be removed. Convert it to a sequence ' +
-      '/ iterable of keyed ReactElements instead.'
-    );
-    console.error.calls.reset();
-
-    var mappedChildren = React.Children.map(instance.props.children, callback, context);
-    assertCalls();
-    expect(mappedChildren).toEqual([
-      <div key=".$#1:0" />,
-      <div key=".$#2:0" />,
-      <div key=".$#3:0" />,
-    ]);
-    expectDev(console.error.calls.count()).toBe(0);
-  });
-
   it('should not enumerate enumerable numbers (#4776)', () => {
     /*eslint-disable no-extend-native */
     Number.prototype['@@iterator'] = function() {

src/renderers/shared/fiber/ReactChildFiber.js

@@ -42,6 +42,7 @@ if (__DEV__) {
   var { getCurrentFiberStackAddendum } = require('ReactDebugCurrentFiber');
   var { getComponentName } = require('ReactFiberTreeReflection');
   var warning = require('warning');
+  var didWarnAboutMaps = false;
 }
 
 const {
@@ -811,6 +812,29 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
     );
 
     if (__DEV__) {
+      // Warn about using Maps as children
+      if (typeof newChildrenIterable.entries === 'function') {
+        const possibleMap = (newChildrenIterable : any);
+        if (possibleMap.entries === iteratorFn) {
+          let mapsAsChildrenAddendum = '';
+          const owner = ReactCurrentOwner.owner || returnFiber._debugOwner;
+          if (owner && typeof owner.tag === 'number') {
+            const mapsAsChildrenOwnerName = getComponentName((owner : any));
+            if (mapsAsChildrenOwnerName) {
+              mapsAsChildrenAddendum = '\n\nCheck the render method of `' + mapsAsChildrenOwnerName + '`.';
+            }
+          }
+          warning(
+            didWarnAboutMaps,
+            'Using Maps as children is unsupported and will likely yield ' +
+            'unexpected results. Convert it to a sequence/iterable of keyed ' +
+            'ReactElements instead.%s',
+            mapsAsChildrenAddendum
+          );
+          didWarnAboutMaps = true;
+        }
+      }
+
       // First, validate keys.
       // We'll get a different iterator later for the main pass.
       const newChildren = iteratorFn.call(newChildrenIterable);

src/renderers/shared/shared/__tests__/ReactMultiChild-test.js

@@ -261,28 +261,25 @@ describe('ReactMultiChild', () => {
         '    in Parent (at **)'
       );
     });
+  });
 
-    it('should warn for using maps as children with owner info', () => {
-      spyOn(console, 'error');
-
-      class Parent extends React.Component {
-        render() {
-          return (
-            <div>{new Map([['foo', 0], ['bar', 1]])}</div>
-          );
-        }
+  it('should warn for using maps as children with owner info', () => {
+    spyOn(console, 'error');
+    class Parent extends React.Component {
+      render() {
+        return (
+          <div>{new Map([['foo', 0], ['bar', 1]])}</div>
+        );
       }
-
-      var container = document.createElement('div');
-      ReactDOM.render(<Parent />, container);
-
-      expectDev(console.error.calls.count()).toBe(1);
-      expectDev(console.error.calls.argsFor(0)[0]).toBe(
-        'Warning: Using Maps as children is not yet fully supported. It is an ' +
-        'experimental feature that might be removed. Convert it to a sequence ' +
-        '/ iterable of keyed ReactElements instead.\n\nCheck the render method of `Parent`.'
-      );
-    });
+    }
+    var container = document.createElement('div');
+    ReactDOM.render(<Parent />, container);
+    expectDev(console.error.calls.count()).toBe(1);
+    expectDev(console.error.calls.argsFor(0)[0]).toBe(
+      'Warning: Using Maps as children is unsupported and will likely yield ' +
+      'unexpected results. Convert it to a sequence/iterable of keyed ' +
+      'ReactElements instead.\n\nCheck the render method of `Parent`.'
+    );
   });
 
   it('should reorder bailed-out children', () => {

src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js

@@ -220,38 +220,4 @@ describe('ReactMultiChildText', () => {
       ReactTestUtils.renderIntoDocument(<div><h1>{['A', 'B']}</h1></div>);
     }).not.toThrow();
   });
-
-  it('should reorder keyed text nodes', () => {
-    spyOn(console, 'error');
-
-    var container = document.createElement('div');
-    ReactDOM.render(
-      <div>{new Map([['a', 'alpha'], ['b', 'beta']])}</div>,
-      container
-    );
-
-    var childNodes = container.firstChild.childNodes;
-    var alpha1 = childNodes[0];
-    var alpha2 = childNodes[1];
-    var alpha3 = childNodes[2];
-    var beta1 = childNodes[3];
-    var beta2 = childNodes[4];
-    var beta3 = childNodes[5];
-
-    ReactDOM.render(
-      <div>{new Map([['b', 'beta'], ['a', 'alpha']])}</div>,
-      container
-    );
-
-    childNodes = container.firstChild.childNodes;
-    expect(childNodes[0]).toBe(beta1);
-    expect(childNodes[1]).toBe(beta2);
-    expect(childNodes[2]).toBe(beta3);
-    expect(childNodes[3]).toBe(alpha1);
-    expect(childNodes[4]).toBe(alpha2);
-    expect(childNodes[5]).toBe(alpha3);
-
-    // Using Maps as children gives a single warning
-    expectDev(console.error.calls.count()).toBe(1);
-  });
 });

src/shared/utils/traverseAllChildren.js

@@ -33,7 +33,7 @@ var SUBSEPARATOR = ':';
  * pattern.
  */
 
-var didWarnAboutMaps = false;
+ var didWarnAboutMaps = false;
 
 /**
  * Generate a key string that identifies a component within a set.
@@ -109,23 +109,10 @@ function traverseAllChildrenImpl(
   } else {
     var iteratorFn = getIteratorFn(children);
     if (iteratorFn) {
-      var iterator = iteratorFn.call(children);
-      var step;
-      if (iteratorFn !== children.entries) {
-        var ii = 0;
-        while (!(step = iterator.next()).done) {
-          child = step.value;
-          nextName = nextNamePrefix + getComponentKey(child, ii++);
-          subtreeCount += traverseAllChildrenImpl(
-            child,
-            nextName,
-            callback,
-            traverseContext
-          );
-        }
-      } else {
-        if (__DEV__) {
-          var mapsAsChildrenAddendum = '';
+      if (__DEV__) {
+        // Warn about using Maps as children
+        if (iteratorFn === children.entries) {
+          let mapsAsChildrenAddendum = '';
           if (ReactCurrentOwner.current) {
             var mapsAsChildrenOwnerName = ReactCurrentOwner.current.getName();
             if (mapsAsChildrenOwnerName) {
@@ -134,31 +121,27 @@ function traverseAllChildrenImpl(
           }
           warning(
             didWarnAboutMaps,
-            'Using Maps as children is not yet fully supported. It is an ' +
-            'experimental feature that might be removed. Convert it to a ' +
-            'sequence / iterable of keyed ReactElements instead.%s',
+            'Using Maps as children is unsupported and will likely yield ' +
+            'unexpected results. Convert it to a sequence/iterable of keyed ' +
+            'ReactElements instead.%s',
             mapsAsChildrenAddendum
           );
           didWarnAboutMaps = true;
         }
-        // Iterator will provide entry [k,v] tuples rather than values.
-        while (!(step = iterator.next()).done) {
-          var entry = step.value;
-          if (entry) {
-            child = entry[1];
-            nextName = (
-              nextNamePrefix +
-              KeyEscapeUtils.escape(entry[0]) + SUBSEPARATOR +
-              getComponentKey(child, 0)
-            );
-            subtreeCount += traverseAllChildrenImpl(
-              child,
-              nextName,
-              callback,
-              traverseContext
-            );
-          }
-        }
+      }
+
+      var iterator = iteratorFn.call(children);
+      var step;
+      var ii = 0;
+      while (!(step = iterator.next()).done) {
+        child = step.value;
+        nextName = nextNamePrefix + getComponentKey(child, ii++);
+        subtreeCount += traverseAllChildrenImpl(
+          child,
+          nextName,
+          callback,
+          traverseContext
+        );
       }
     } else if (type === 'object') {
       var addendum = '';