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;