Merge pull request #8677 from bvaughn/add-callback-validation-to-dom-…

…render

Add callback validation to fiber-based renderers

scripts/fiber/tests-failing.txt

@@ -12,10 +12,6 @@ src/isomorphic/classic/__tests__/ReactContextValidator-test.js
 src/renderers/dom/__tests__/ReactDOMProduction-test.js
 * should throw with an error code in production
 
-src/renderers/dom/shared/__tests__/ReactDOM-test.js
-* throws in render() if the mount callback is not a function
-* throws in render() if the update callback is not a function
-
 src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
 * should clean up input value tracking
 * should clean up input textarea tracking

scripts/fiber/tests-passing.txt

@@ -620,6 +620,8 @@ src/renderers/dom/shared/__tests__/ReactDOM-test.js
 * should overwrite props.children with children argument
 * should purge the DOM cache when removing nodes
 * allow React.DOM factories to be called without warnings
+* throws in render() if the mount callback is not a function
+* throws in render() if the update callback is not a function
 * preserves focus
 * calls focus() on autoFocus elements after they have been mounted to the DOM
 

src/renderers/dom/shared/__tests__/ReactDOM-test.js

@@ -133,15 +133,15 @@ describe('ReactDOM', () => {
 
     var myDiv = document.createElement('div');
     expect(() => ReactDOM.render(<A />, myDiv, 'no')).toThrowError(
-      'ReactDOM.render(...): Expected the last optional `callback` argument ' +
+      'render(...): Expected the last optional `callback` argument ' +
       'to be a function. Instead received: string.'
     );
     expect(() => ReactDOM.render(<A />, myDiv, {})).toThrowError(
-      'ReactDOM.render(...): Expected the last optional `callback` argument ' +
+      'render(...): Expected the last optional `callback` argument ' +
       'to be a function. Instead received: Object.'
     );
     expect(() => ReactDOM.render(<A />, myDiv, new Foo())).toThrowError(
-      'ReactDOM.render(...): Expected the last optional `callback` argument ' +
+      'render(...): Expected the last optional `callback` argument ' +
       'to be a function. Instead received: Foo (keys: a, b).'
     );
   });
@@ -164,15 +164,15 @@ describe('ReactDOM', () => {
     ReactDOM.render(<A />, myDiv);
 
     expect(() => ReactDOM.render(<A />, myDiv, 'no')).toThrowError(
-      'ReactDOM.render(...): Expected the last optional `callback` argument ' +
+      'render(...): Expected the last optional `callback` argument ' +
       'to be a function. Instead received: string.'
     );
     expect(() => ReactDOM.render(<A />, myDiv, {})).toThrowError(
-      'ReactDOM.render(...): Expected the last optional `callback` argument ' +
+      'render(...): Expected the last optional `callback` argument ' +
       'to be a function. Instead received: Object.'
     );
     expect(() => ReactDOM.render(<A />, myDiv, new Foo())).toThrowError(
-      'ReactDOM.render(...): Expected the last optional `callback` argument ' +
+      'render(...): Expected the last optional `callback` argument ' +
       'to be a function. Instead received: Foo (keys: a, b).'
     );
   });

src/renderers/dom/stack/client/ReactMount.js

@@ -33,6 +33,7 @@ var invariant = require('invariant');
 var setInnerHTML = require('setInnerHTML');
 var shouldUpdateReactComponent = require('shouldUpdateReactComponent');
 var warning = require('warning');
+var validateCallback = require('validateCallback');
 
 var ATTR_NAME = DOMProperty.ID_ATTRIBUTE_NAME;
 var ROOT_ATTR_NAME = DOMProperty.ROOT_ATTRIBUTE_NAME;
@@ -432,7 +433,7 @@ var ReactMount = {
   },
 
   _renderSubtreeIntoContainer: function(parentComponent, nextElement, container, callback) {
-    ReactUpdateQueue.validateCallback(callback, 'ReactDOM.render');
+    validateCallback(callback, 'ReactDOM.render');
     invariant(
       React.isValidElement(nextElement),
       'ReactDOM.render(): Invalid component element.%s',

src/renderers/shared/fiber/ReactFiberClassComponent.js

@@ -33,31 +33,6 @@ var invariant = require('invariant');
 
 const isArray = Array.isArray;
 
-function formatUnexpectedArgument(arg) {
-  var type = typeof arg;
-  if (type !== 'object') {
-    return type;
-  }
-  var displayName = arg.constructor && arg.constructor.name || type;
-  var keys = Object.keys(arg);
-  if (keys.length > 0 && keys.length < 20) {
-    return `${displayName} (keys: ${keys.join(', ')})`;
-  }
-  return displayName;
-}
-
-function validateCallback(callback, callerName) {
-  if (typeof callback !== 'function') {
-    invariant(
-      false,
-      '%s(...): Expected the last optional `callback` argument to be a ' +
-      'function. Instead received: %s.',
-      callerName,
-      formatUnexpectedArgument(callback)
-    );
-  }
-}
-
 module.exports = function(
   scheduleUpdate : (fiber : Fiber, priorityLevel : PriorityLevel) => void,
   getPriorityContext : () => PriorityLevel,
@@ -67,27 +42,18 @@ module.exports = function(
   const updater = {
     isMounted,
     enqueueSetState(instance, partialState, callback) {
-      if (callback) {
-        validateCallback(callback, 'setState');
-      }
       const fiber = ReactInstanceMap.get(instance);
       const priorityLevel = getPriorityContext();
       addUpdate(fiber, partialState, callback || null, priorityLevel);
       scheduleUpdate(fiber, priorityLevel);
     },
     enqueueReplaceState(instance, state, callback) {
-      if (callback) {
-        validateCallback(callback, 'replaceState');
-      }
       const fiber = ReactInstanceMap.get(instance);
       const priorityLevel = getPriorityContext();
       addReplaceUpdate(fiber, state, callback || null, priorityLevel);
       scheduleUpdate(fiber, priorityLevel);
     },
     enqueueForceUpdate(instance, callback) {
-      if (callback) {
-        validateCallback(callback, 'forceUpdate');
-      }
       const fiber = ReactInstanceMap.get(instance);
       const priorityLevel = getPriorityContext();
       addForceUpdate(fiber, callback || null, priorityLevel);

src/renderers/shared/fiber/ReactFiberUpdateQueue.js

@@ -25,6 +25,7 @@ const {
   TaskPriority,
 } = require('ReactPriorityLevel');
 
+const validateCallback = require('validateCallback');
 const warning = require('warning');
 
 type PartialState<State, Props> =
@@ -204,12 +205,14 @@ function findInsertionPosition(queue, update) : Update | null {
 // we shouldn't make a copy.
 //
 // If the update is cloned, it returns the cloned update.
-function insertUpdate(fiber : Fiber, update : Update, methodName : ?string) : Update | null {
+function insertUpdate(fiber : Fiber, update : Update, methodName : string) : Update | null {
+  validateCallback(update.callback, methodName);
+
   const queue1 = ensureUpdateQueue(fiber);
   const queue2 = fiber.alternate ? ensureUpdateQueue(fiber.alternate) : null;
 
   // Warn if an update is scheduled from inside an updater function.
-  if (__DEV__ && typeof methodName === 'string') {
+  if (__DEV__) {
     if (queue1.isProcessing || (queue2 && queue2.isProcessing)) {
       if (methodName === 'setState') {
         warning(
@@ -287,11 +290,7 @@ function addUpdate(
     isTopLevelUnmount: false,
     next: null,
   };
-  if (__DEV__) {
-    insertUpdate(fiber, update, 'setState');
-  } else {
-    insertUpdate(fiber, update);
-  }
+  insertUpdate(fiber, update, 'setState');
 }
 exports.addUpdate = addUpdate;
 
@@ -310,12 +309,7 @@ function addReplaceUpdate(
     isTopLevelUnmount: false,
     next: null,
   };
-
-  if (__DEV__) {
-    insertUpdate(fiber, update, 'replaceState');
-  } else {
-    insertUpdate(fiber, update);
-  }
+  insertUpdate(fiber, update, 'replaceState');
 }
 exports.addReplaceUpdate = addReplaceUpdate;
 
@@ -333,11 +327,7 @@ function addForceUpdate(
     isTopLevelUnmount: false,
     next: null,
   };
-  if (__DEV__) {
-    insertUpdate(fiber, update, 'forceUpdate');
-  } else {
-    insertUpdate(fiber, update);
-  }
+  insertUpdate(fiber, update, 'forceUpdate');
 }
 exports.addForceUpdate = addForceUpdate;
 
@@ -366,7 +356,7 @@ function addTopLevelUpdate(
     isTopLevelUnmount,
     next: null,
   };
-  const update2 = insertUpdate(fiber, update);
+  const update2 = insertUpdate(fiber, update, 'render');
 
   if (isTopLevelUnmount) {
     // Drop all updates that are lower-priority, so that the tree is not

src/renderers/shared/stack/reconciler/ReactUpdateQueue.js

@@ -16,26 +16,13 @@ var ReactInstanceMap = require('ReactInstanceMap');
 var ReactInstrumentation = require('ReactInstrumentation');
 var ReactUpdates = require('ReactUpdates');
 
-var invariant = require('invariant');
 var warning = require('warning');
+var validateCallback = require('validateCallback');
 
 function enqueueUpdate(internalInstance) {
   ReactUpdates.enqueueUpdate(internalInstance);
 }
 
-function formatUnexpectedArgument(arg) {
-  var type = typeof arg;
-  if (type !== 'object') {
-    return type;
-  }
-  var displayName = arg.constructor && arg.constructor.name || type;
-  var keys = Object.keys(arg);
-  if (keys.length > 0 && keys.length < 20) {
-    return `${displayName} (keys: ${keys.join(', ')})`;
-  }
-  return displayName;
-}
-
 function getInternalInstanceReadyForUpdate(publicInstance, callerName) {
   var internalInstance = ReactInstanceMap.get(publicInstance);
   if (!internalInstance) {
@@ -147,7 +134,7 @@ var ReactUpdateQueue = {
     }
 
     if (callback) {
-      ReactUpdateQueue.validateCallback(callback, callerName);
+      validateCallback(callback, callerName);
       if (internalInstance._pendingCallbacks) {
         internalInstance._pendingCallbacks.push(callback);
       } else {
@@ -187,7 +174,7 @@ var ReactUpdateQueue = {
     internalInstance._pendingReplaceState = true;
 
     if (callback) {
-      ReactUpdateQueue.validateCallback(callback, callerName);
+      validateCallback(callback, callerName);
       if (internalInstance._pendingCallbacks) {
         internalInstance._pendingCallbacks.push(callback);
       } else {
@@ -235,7 +222,7 @@ var ReactUpdateQueue = {
     queue.push(partialState);
 
     if (callback) {
-      ReactUpdateQueue.validateCallback(callback, callerName);
+      validateCallback(callback, callerName);
       if (internalInstance._pendingCallbacks) {
         internalInstance._pendingCallbacks.push(callback);
       } else {
@@ -253,16 +240,6 @@ var ReactUpdateQueue = {
     enqueueUpdate(internalInstance);
   },
 
-  validateCallback: function(callback, callerName) {
-    invariant(
-      !callback || typeof callback === 'function',
-      '%s(...): Expected the last optional `callback` argument to be a ' +
-      'function. Instead received: %s.',
-      callerName,
-      formatUnexpectedArgument(callback)
-    );
-  },
-
 };
 
 module.exports = ReactUpdateQueue;

src/renderers/shared/utils/validateCallback.js

@@ -0,0 +1,40 @@
+/**
+ * Copyright 2013-present, 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 validateCallback
+ * @flow
+ */
+
+'use strict';
+
+const invariant = require('invariant');
+
+function formatUnexpectedArgument(arg: any) {
+  let type = typeof arg;
+  if (type !== 'object') {
+    return type;
+  }
+  let displayName = arg.constructor && arg.constructor.name || type;
+  let keys = Object.keys(arg);
+  if (keys.length > 0 && keys.length < 20) {
+    return `${displayName} (keys: ${keys.join(', ')})`;
+  }
+  return displayName;
+}
+
+function validateCallback(callback: ?Function, callerName: string) {
+  invariant(
+    !callback || typeof callback === 'function',
+    '%s(...): Expected the last optional `callback` argument to be a ' +
+    'function. Instead received: %s.',
+    callerName,
+    formatUnexpectedArgument(callback)
+  );
+}
+
+module.exports = validateCallback;