Merge pull request #3171 from sebmarkbage/moarwarnings

Moar Warnings

src/browser/__tests__/ReactDOM-test.js

@@ -118,4 +118,5 @@ describe('ReactDOM', function() {
     expect(element.type).toBe('div');
     expect(console.warn.argsForCall.length).toBe(0);
   });
+
 });

src/browser/findDOMNode.js

@@ -11,11 +11,14 @@
  */
 
 'use strict';
+
+var ReactCurrentOwner = require('ReactCurrentOwner');
 var ReactInstanceMap = require('ReactInstanceMap');
 var ReactMount = require('ReactMount');
 
 var invariant = require('invariant');
 var isNode = require('isNode');
+var warning = require('warning');
 
 /**
  * Returns the DOM node rendered by this element.
@@ -24,6 +27,21 @@ var isNode = require('isNode');
  * @return {DOMElement} The root node of this element.
  */
 function findDOMNode(componentOrElement) {
+  if (__DEV__) {
+    var owner = ReactCurrentOwner.current;
+    if (owner !== null) {
+      warning(
+        owner._warnedAboutRefsInRender,
+        '%s is accessing getDOMNode or findDOMNode inside its render(). ' +
+        'render() should be a pure function of props and state. It should ' +
+        'never access something that requires stale data from the previous ' +
+        'render, such as refs. Move this logic to componentDidMount and ' +
+        'componentDidUpdate instead.',
+        owner.getName() || 'A component'
+      );
+      owner._warnedAboutRefsInRender = true;
+    }
+  }
   if (componentOrElement == null) {
     return null;
   }

src/classic/class/ReactClass.js

@@ -12,6 +12,7 @@
 'use strict';
 
 var ReactComponent = require('ReactComponent');
+var ReactCurrentOwner = require('ReactCurrentOwner');
 var ReactElement = require('ReactElement');
 var ReactErrorUtils = require('ReactErrorUtils');
 var ReactInstanceMap = require('ReactInstanceMap');
@@ -746,6 +747,21 @@ var ReactClassMixin = {
    * @final
    */
   isMounted: function() {
+    if (__DEV__) {
+      var owner = ReactCurrentOwner.current;
+      if (owner !== null) {
+        warning(
+          owner._warnedAboutRefsInRender,
+          '%s is accessing isMounted inside its render() function. ' +
+          'render() should be a pure function of props and state. It should ' +
+          'never access something that requires stale data from the previous ' +
+          'render, such as refs. Move this logic to componentDidMount and ' +
+          'componentDidUpdate instead.',
+          owner.getName() || 'A component'
+        );
+        owner._warnedAboutRefsInRender = true;
+      }
+    }
     var internalInstance = ReactInstanceMap.get(this);
     return (
       internalInstance &&

src/classic/element/ReactElementValidator.js

@@ -387,6 +387,33 @@ var ReactElementValidator = {
     );
     // Legacy hook TODO: Warn if this is accessed
     validatedFactory.type = type;
+
+    if (__DEV__) {
+      try {
+        Object.defineProperty(
+          validatedFactory,
+          'type',
+          {
+            enumerable: false,
+            get: function() {
+              warning(
+                false,
+                'Factory.type is deprecated. Access the class directly ' +
+                'before passing it to createFactory.'
+              );
+              Object.defineProperty(this, 'type', {
+                value: type
+              });
+              return type;
+            }
+          }
+        );
+      } catch (x) {
+        // IE will fail on defineProperty (es5-shim/sham too)
+      }
+    }
+
+
     return validatedFactory;
   }
 

src/classic/element/__tests__/ReactElementValidator-test.js

@@ -330,4 +330,23 @@ describe('ReactElementValidator', function() {
     expect(console.warn.calls[0].args[0]).toContain('use of a keyed object');
   });
 
+  it('should warn when accessing .type on an element factory', function() {
+    spyOn(console, 'warn');
+    var TestComponent = React.createClass({
+      render: function() {
+        return <div />;
+      }
+    });
+    var TestFactory = React.createFactory(TestComponent);
+    expect(TestFactory.type).toBe(TestComponent);
+    expect(console.warn.argsForCall.length).toBe(1);
+    expect(console.warn.argsForCall[0][0]).toBe(
+      'Warning: Factory.type is deprecated. Access the class directly before ' +
+      'passing it to createFactory.'
+    );
+    // Warn once, not again
+    expect(TestFactory.type).toBe(TestComponent);
+    expect(console.warn.argsForCall.length).toBe(1);
+  });
+
 });

src/core/__tests__/ReactComponent-test.js

@@ -254,26 +254,6 @@ describe('ReactComponent', function() {
     ]);
   });
 
-  it('should correctly determine if a component is mounted', function() {
-    var Component = React.createClass({
-      componentWillMount: function() {
-        expect(this.isMounted()).toBeFalsy();
-      },
-      componentDidMount: function() {
-        expect(this.isMounted()).toBeTruthy();
-      },
-      render: function() {
-        expect(this.isMounted()).toBeFalsy()
-        return <div/>;
-      }
-    });
-
-    var element = <Component />;
-
-    var instance = ReactTestUtils.renderIntoDocument(element);
-    expect(instance.isMounted()).toBeTruthy();
-  });
-
   it('fires the callback after a component is rendered', function() {
     var callback = mocks.getMockFunction();
     var container = document.createElement('div');

src/core/__tests__/ReactComponentLifeCycle-test.js

@@ -261,16 +261,54 @@ describe('ReactComponentLifeCycle', function() {
     );
   });
 
-  it('is not mounted inside initial render', function() {
-    var InitialRender = React.createClass({
+  it('should correctly determine if a component is mounted', function() {
+    spyOn(console, 'warn');
+    var Component = React.createClass({
+      componentWillMount: function() {
+        expect(this.isMounted()).toBeFalsy();
+      },
+      componentDidMount: function() {
+        expect(this.isMounted()).toBeTruthy();
+      },
       render: function() {
-        expect(this.isMounted()).toBe(false);
-        return (
-          <div></div>
-        );
+        expect(this.isMounted()).toBeFalsy()
+        return <div/>;
       }
     });
-    ReactTestUtils.renderIntoDocument(<InitialRender />);
+
+    var element = <Component />;
+
+    var instance = ReactTestUtils.renderIntoDocument(element);
+    expect(instance.isMounted()).toBeTruthy();
+
+    expect(console.warn.argsForCall.length).toBe(1);
+    expect(console.warn.argsForCall[0][0]).toContain(
+      'Component is accessing isMounted inside its render()'
+    );
+  });
+
+  it('warns if getDOMNode is used inside render', function() {
+    spyOn(console, 'warn');
+    var Component = React.createClass({
+      getInitialState: function() {
+        return { isMounted: false };
+      },
+      componentDidMount: function() {
+        this.setState({ isMounted: true });
+      },
+      render: function() {
+        if (this.state.isMounted) {
+          expect(this.getDOMNode().tagName).toBe('DIV');
+        }
+        return <div/>;
+      }
+    });
+
+    ReactTestUtils.renderIntoDocument(<Component />);
+    expect(console.warn.argsForCall.length).toBe(1);
+    expect(console.warn.argsForCall[0][0]).toContain(
+      'Component is accessing getDOMNode or findDOMNode inside its render()'
+    );
   });
 
   it('should carry through each of the phases of setup', function() {

src/core/instantiateReactComponent.js

@@ -116,6 +116,7 @@ function instantiateReactComponent(node, parentCompositeType) {
 
   if (__DEV__) {
     instance._isOwnerNecessary = false;
+    instance._warnedAboutRefsInRender = false;
   }
 
   // Internal instances should fully constructed at this point, so they should

src/utils/__tests__/traverseAllChildren-test.js

@@ -396,6 +396,8 @@ describe('traverseAllChildren', function() {
   });
 
   it('should use keys from entry iterables', function() {
+    spyOn(console, 'warn');
+
     var threeDivEntryIterable = {
       '@@iterator': function() {
         var i = 0;
@@ -445,6 +447,13 @@ describe('traverseAllChildren', function() {
       '.$#3:0',
       2
     );
+
+    expect(console.warn.argsForCall.length).toBe(1);
+    expect(console.warn.argsForCall[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.'
+    );
   });
 
 });

src/utils/traverseAllChildren.js

@@ -17,6 +17,7 @@ var ReactInstanceHandles = require('ReactInstanceHandles');
 
 var getIteratorFn = require('getIteratorFn');
 var invariant = require('invariant');
+var warning = require('warning');
 
 var SEPARATOR = ReactInstanceHandles.SEPARATOR;
 var SUBSEPARATOR = ':';
@@ -34,6 +35,8 @@ var userProvidedKeyEscaperLookup = {
 
 var userProvidedKeyEscapeRegex = /[=.:]/g;
 
+var didWarnAboutMaps = false;
+
 function userProvidedKeyEscaper(match) {
   return userProvidedKeyEscaperLookup[match];
 }
@@ -158,6 +161,15 @@ function traverseAllChildrenImpl(
           );
         }
       } else {
+        if (__DEV__) {
+          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.'
+          );
+          didWarnAboutMaps = true;
+        }
         // Iterator will provide entry [k,v] tuples rather than values.
         while (!(step = iterator.next()).done) {
           var entry = step.value;