Make keyed object fragments an opaque type

This triggers a warning if you try to pass a keyed object as a child. You now have to wrap it in React.addons.createFragment(object) which creates a proxy (in dev) which warns if it is accessed. The purpose of this is to make these into opaque objects so that nobody relies on its data structure. After that we can turn it into a different data structure such as a ReactFragment node or an iterable of flattened ReactElements.
facebook · Feb 9, 2015 · 56f5115 · 56f5115
1 parent 44634c0
commit 56f5115
Show file tree

Hide file tree

Showing 19 changed files with 468 additions and 120 deletions.
diff --git a/src/addons/ReactFragment.js b/src/addons/ReactFragment.js
@@ -0,0 +1,164 @@
+/**
+ * Copyright 2015, Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD-style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ *
+* @providesModule ReactFragment
+*/
+
+'use strict';
+
+var ReactElement = require('ReactElement');
+
+var warning = require('warning');
+
+/**
+ * We used to allow keyed objects to serve as a collection of ReactElements,
+ * or nested sets. This allowed us a way to explicitly key a set a fragment of
+ * components. This is now being replaced with an opaque data structure.
+ * The upgrade path is to call React.addons.createFragment({ key: value }) to
+ * create a keyed fragment. The resulting data structure is opaque, for now.
+ */
+
+if (__DEV__) {
+  var fragmentKey = '_reactFragment';
+  var didWarnKey = '_reactDidWarn';
+  var canWarnForReactFragment = false;
+
+  try {
+    // Feature test. Don't even try to issue this warning if we can't use
+    // enumerable: false.
+
+    var dummy = function() {
+      return 1;
+    };
+
+    Object.defineProperty(
+      {},
+      fragmentKey,
+      { enumerable: false, value: true }
+    );
+
+    Object.defineProperty(
+      {},
+      'key',
+      { enumerable: true, get: dummy }
+    );
+
+    canWarnForReactFragment = true;
+  } catch (x) { }
+
+  var proxyPropertyAccessWithWarning = function(obj, key) {
+    Object.defineProperty(obj, key, {
+      enumerable: true,
+      get: function() {
+        warning(
+          this[didWarnKey],
+          'A ReactFragment is an opaque type. Accessing any of its ' +
+          'properties is deprecated. Pass it to one of the React.Children ' +
+          'helpers.'
+        );
+        this[didWarnKey] = true;
+        return this[fragmentKey][key];
+      },
+      set: function(value) {
+        warning(
+          this[didWarnKey],
+          'A ReactFragment is an immutable opaque type. Mutating its ' +
+          'properties is deprecated.'
+        );
+        this[didWarnKey] = true;
+        this[fragmentKey][key] = value;
+      }
+    });
+  };
+
+  var issuedWarnings = {};
+
+  var didWarnForFragment = function(fragment) {
+    // We use the keys and the type of the value as a heuristic to dedupe the
+    // warning to avoid spamming too much.
+    var fragmentCacheKey = '';
+    for (var key in fragment) {
+      fragmentCacheKey += key + ':' + (typeof fragment[key]) + ',';
+    }
+    var alreadyWarnedOnce = !!issuedWarnings[fragmentCacheKey];
+    issuedWarnings[fragmentCacheKey] = true;
+    return alreadyWarnedOnce;
+  };
+}
+
+var ReactFragment = {
+  // Wrap a keyed object in an opaque proxy that warns you if you access any
+  // of its properties.
+  create: function(object) {
+    if (__DEV__) {
+      if (canWarnForReactFragment) {
+        var proxy = {};
+        Object.defineProperty(proxy, fragmentKey, {
+          enumerable: false,
+          value: object
+        });
+        Object.defineProperty(proxy, didWarnKey, {
+          writable: true,
+          enumerable: false,
+          value: false
+        });
+        for (var key in object) {
+          proxyPropertyAccessWithWarning(proxy, key);
+        }
+        Object.preventExtensions(proxy);
+        return proxy;
+      }
+    }
+    return object;
+  },
+  // Extract the original keyed object from the fragment opaque type. Warn if
+  // a plain object is passed here.
+  extract: function(fragment) {
+    if (__DEV__) {
+      if (canWarnForReactFragment) {
+        if (!fragment[fragmentKey]) {
+          warning(
+            didWarnForFragment(fragment),
+            'Any use of a keyed object should be wrapped in ' +
+            'React.addons.createFragment(object) before passed as a child.'
+          );
+          return fragment;
+        }
+        return fragment[fragmentKey];
+      }
+    }
+    return fragment;
+  },
+  // Check if this is a fragment and if so, extract the keyed object. If it
+  // is a fragment-like object, warn that it should be wrapped. Ignore if we
+  // can't determine what kind of object this is.
+  extractIfFragment: function(fragment) {
+    if (__DEV__) {
+      if (canWarnForReactFragment) {
+        // If it is the opaque type, return the keyed object.
+        if (fragment[fragmentKey]) {
+          return fragment[fragmentKey];
+        }
+        // Otherwise, check each property if it has an element, if it does
+        // it is probably meant as a fragment, so we can warn early. Defer,
+        // the warning to extract.
+        for (var key in fragment) {
+          if (fragment.hasOwnProperty(key) &&
+              ReactElement.isValidElement(fragment[key])) {
+            // This looks like a fragment object, we should provide an
+            // early warning.
+            return ReactFragment.extract(fragment);
+          }
+        }
+      }
+    }
+    return fragment;
+  }
+};
+
+module.exports = ReactFragment;
diff --git a/src/addons/__tests__/ReactFragment-test.js b/src/addons/__tests__/ReactFragment-test.js
@@ -0,0 +1,76 @@
+/**
+ * Copyright 2015, Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD-style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ *
+ * @emails react-core
+ */
+
+'use strict';
+
+var React;
+var ReactFragment;
+
+describe('ReactFragment', function() {
+
+  beforeEach(function() {
+    React = require('React');
+    ReactFragment = require('ReactFragment');
+  });
+
+  it('should warn if a plain object is used as a child', function() {
+    spyOn(console, 'warn');
+    var children = {
+      x: <span />,
+      y: <span />
+    };
+    <div>{children}</div>;
+    expect(console.warn.calls.length).toBe(1);
+    expect(console.warn.calls[0].args[0]).toContain(
+      'Any use of a keyed object'
+    );
+    // Only warn once for the same set of children
+    var sameChildren = {
+      x: <span />,
+      y: <span />
+    };
+    <div>{sameChildren}</div>;
+    expect(console.warn.calls.length).toBe(1);
+  });
+
+  it('should warn if a plain object even if it is deep', function() {
+    spyOn(console, 'warn');
+    var children = {
+      x: <span />,
+      y: <span />,
+      z: <span />
+    };
+    var element = <div>{[children]}</div>;
+    expect(console.warn.calls.length).toBe(0);
+    var container = document.createElement('div');
+    React.render(element, container);
+    expect(console.warn.calls.length).toBe(1);
+    expect(console.warn.calls[0].args[0]).toContain(
+      'Any use of a keyed object'
+    );
+  });
+
+  it('should warn if accessing any property on a fragment', function() {
+    spyOn(console, 'warn');
+    var children = {
+      x: <span />,
+      y: <span />
+    };
+    var frag = ReactFragment.create(children);
+    frag.x;
+    frag.y = 10;
+    expect(console.warn.calls.length).toBe(1);
+    expect(console.warn.calls[0].args[0]).toContain(
+      'A ReactFragment is an opaque type'
+    );
+  });
+
+});
diff --git a/src/addons/transitions/ReactTransitionChildMapping.js b/src/addons/transitions/ReactTransitionChildMapping.js
@@ -13,6 +13,7 @@
 'use strict';
 
 var ReactChildren = require('ReactChildren');
+var ReactFragment = require('ReactFragment');
 
 var ReactTransitionChildMapping = {
   /**
@@ -23,9 +24,12 @@ var ReactTransitionChildMapping = {
    * @return {object} Mapping of key to child
    */
   getChildMapping: function(children) {
-    return ReactChildren.map(children, function(child) {
+    if (!children) {
+      return children;
+    }
+    return ReactFragment.extract(ReactChildren.map(children, function(child) {
       return child;
-    });
+    }));
   },
 
   /**

diff --git a/src/addons/transitions/ReactTransitionGroup.js b/src/addons/transitions/ReactTransitionGroup.js
@@ -202,7 +202,7 @@ var ReactTransitionGroup = React.createClass({
   render: function() {
     // TODO: we could get rid of the need for the wrapper node
     // by cloning a single child
-    var childrenToRender = {};
+    var childrenToRender = [];
     for (var key in this.state.children) {
       var child = this.state.children[key];
       if (child) {
@@ -211,10 +211,10 @@ var ReactTransitionGroup = React.createClass({
         // already been removed. In case you need this behavior you can provide
         // a childFactory function to wrap every child, even the ones that are
         // leaving.
-        childrenToRender[key] = cloneWithProps(
+        childrenToRender.push(cloneWithProps(
           this.props.childFactory(child),
-          {ref: key}
-        );
+          {ref: key, key: key}
+        ));
       }
     }
     return React.createElement(

diff --git a/src/browser/ReactWithAddons.js b/src/browser/ReactWithAddons.js
@@ -23,6 +23,7 @@ var React = require('React');
 var ReactComponentWithPureRenderMixin =
   require('ReactComponentWithPureRenderMixin');
 var ReactCSSTransitionGroup = require('ReactCSSTransitionGroup');
+var ReactFragment = require('ReactFragment');
 var ReactTransitionGroup = require('ReactTransitionGroup');
 var ReactUpdates = require('ReactUpdates');
 
@@ -39,6 +40,7 @@ React.addons = {
   batchedUpdates: ReactUpdates.batchedUpdates,
   classSet: cx,
   cloneWithProps: cloneWithProps,
+  createFragment: ReactFragment.create,
   update: update
 };
 

diff --git a/src/browser/__tests__/ReactDOM-test.js b/src/browser/__tests__/ReactDOM-test.js
@@ -75,37 +75,37 @@ describe('ReactDOM', function() {
    */
   it("should purge the DOM cache when removing nodes", function() {
     var myDiv = ReactTestUtils.renderIntoDocument(
-      <div>{{
-        theDog: <div className="dog" />,
-        theBird: <div className="bird" />
-      }}</div>
+      <div>
+        <div key="theDog" className="dog" />,
+        <div key="theBird" className="bird" />
+      </div>
     );
     // Warm the cache with theDog
     myDiv.setProps({
-      children: {
-        theDog: <div className="dogbeforedelete" />,
-        theBird: <div className="bird" />
-      }
+      children: [
+        <div key="theDog" className="dogbeforedelete" />,
+        <div key="theBird" className="bird" />
+      ]
     });
     // Remove theDog - this should purge the cache
     myDiv.setProps({
-      children: {
-        theBird: <div className="bird" />
-      }
+      children: [
+        <div key="theBird" className="bird" />
+      ]
     });
     // Now, put theDog back. It's now a different DOM node.
     myDiv.setProps({
-      children: {
-        theDog: <div className="dog" />,
-        theBird: <div className="bird" />
-      }
+      children: [
+        <div key="theDog" className="dog" />,
+        <div key="theBird" className="bird" />
+      ]
     });
     // Change the className of theDog. It will use the same element
     myDiv.setProps({
-      children: {
-        theDog: <div className="bigdog" />,
-        theBird: <div className="bird" />
-      }
+      children: [
+        <div key="theDog" className="bigdog" />,
+        <div key="theBird" className="bird" />
+      ]
     });
     var root = myDiv.getDOMNode();
     var dog = root.childNodes[0];