From d3aeca1ff89e2ecedcd9be5ebe0a08336def5048 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 9 Feb 2017 15:35:59 -0800 Subject: [PATCH] Validate callbacks just before they are invoked This delays the type check until the point when the engine will have to perform a type check regardless. Because the error is now thrown during the commit phase rather than when the callback is originally scheduled, we warn in DEV when scheduling so it's easier to track down. --- src/renderers/__tests__/ReactUpdates-test.js | 93 ++++++++++++++----- .../dom/shared/__tests__/ReactDOM-test.js | 61 +++++++++--- src/renderers/dom/stack/client/ReactMount.js | 12 ++- .../shared/fiber/ReactFiberClassComponent.js | 32 ++++++- .../shared/fiber/ReactFiberReconciler.js | 10 ++ .../shared/fiber/ReactFiberUpdateQueue.js | 21 +++-- .../shared/stack/reconciler/CallbackQueue.js | 2 + .../stack/reconciler/ReactUpdateQueue.js | 23 ++++- .../shared/utils/validateCallback.js | 34 ++----- 9 files changed, 208 insertions(+), 80 deletions(-) diff --git a/src/renderers/__tests__/ReactUpdates-test.js b/src/renderers/__tests__/ReactUpdates-test.js index 4a455cc0bb..bbe0fc47cb 100644 --- a/src/renderers/__tests__/ReactUpdates-test.js +++ b/src/renderers/__tests__/ReactUpdates-test.js @@ -901,6 +901,8 @@ describe('ReactUpdates', () => { }); it('throws in setState if the update callback is not a function', () => { + spyOn(console, 'error'); + function Foo() { this.a = 1; this.b = 2; @@ -917,20 +919,37 @@ describe('ReactUpdates', () => { var component = ReactTestUtils.renderIntoDocument(); expect(() => component.setState({}, 'no')).toThrowError( - 'setState(...): Expected the last optional `callback` argument ' + - 'to be a function. Instead received: string.' + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: no', ); - expect(() => component.setState({}, {})).toThrowError( - 'setState(...): Expected the last optional `callback` argument ' + - 'to be a function. Instead received: Object.' + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'setState(...): Expected the last optional `callback` argument to be ' + + 'a function. Instead received: no.' ); + component = ReactTestUtils.renderIntoDocument(); + expect(() => component.setState({}, {foo: 'bar'})).toThrowError( + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: [object Object]', + ); + expectDev(console.error.calls.argsFor(1)[0]).toContain( + 'setState(...): Expected the last optional `callback` argument to be ' + + 'a function. Instead received: [object Object].' + ); + component = ReactTestUtils.renderIntoDocument(); expect(() => component.setState({}, new Foo())).toThrowError( - 'setState(...): Expected the last optional `callback` argument ' + - 'to be a function. Instead received: Foo (keys: a, b).' + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: [object Object]', ); + expectDev(console.error.calls.argsFor(2)[0]).toContain( + 'setState(...): Expected the last optional `callback` argument to be ' + + 'a function. Instead received: [object Object].' + ); + expect(console.error.calls.count()).toBe(3); }); it('throws in replaceState if the update callback is not a function', () => { + spyOn(console, 'error'); + function Foo() { this.a = 1; this.b = 2; @@ -946,20 +965,37 @@ describe('ReactUpdates', () => { var component = ReactTestUtils.renderIntoDocument(); expect(() => component.replaceState({}, 'no')).toThrowError( - 'replaceState(...): Expected the last optional `callback` argument ' + - 'to be a function. Instead received: string.' + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: no', ); - expect(() => component.replaceState({}, {})).toThrowError( - 'replaceState(...): Expected the last optional `callback` argument ' + - 'to be a function. Instead received: Object.' + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'replaceState(...): Expected the last optional `callback` argument to be ' + + 'a function. Instead received: no.' ); + component = ReactTestUtils.renderIntoDocument(); + expect(() => component.replaceState({}, {foo: 'bar'})).toThrowError( + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: [object Object]', + ); + expectDev(console.error.calls.argsFor(1)[0]).toContain( + 'replaceState(...): Expected the last optional `callback` argument to be ' + + 'a function. Instead received: [object Object].' + ); + component = ReactTestUtils.renderIntoDocument(); expect(() => component.replaceState({}, new Foo())).toThrowError( - 'replaceState(...): Expected the last optional `callback` argument ' + - 'to be a function. Instead received: Foo (keys: a, b).' + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: [object Object]', ); + expectDev(console.error.calls.argsFor(2)[0]).toContain( + 'replaceState(...): Expected the last optional `callback` argument to be ' + + 'a function. Instead received: [object Object].' + ); + expect(console.error.calls.count()).toBe(3); }); it('throws in forceUpdate if the update callback is not a function', () => { + spyOn(console, 'error'); + function Foo() { this.a = 1; this.b = 2; @@ -976,17 +1012,32 @@ describe('ReactUpdates', () => { var component = ReactTestUtils.renderIntoDocument(); expect(() => component.forceUpdate('no')).toThrowError( - 'forceUpdate(...): Expected the last optional `callback` argument ' + - 'to be a function. Instead received: string.' + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: no', ); - expect(() => component.forceUpdate({})).toThrowError( - 'forceUpdate(...): Expected the last optional `callback` argument ' + - 'to be a function. Instead received: Object.' + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'forceUpdate(...): Expected the last optional `callback` argument to be ' + + 'a function. Instead received: no.' ); + component = ReactTestUtils.renderIntoDocument(); + expect(() => component.forceUpdate({foo: 'bar'})).toThrowError( + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: [object Object]', + ); + expectDev(console.error.calls.argsFor(1)[0]).toContain( + 'forceUpdate(...): Expected the last optional `callback` argument to be ' + + 'a function. Instead received: [object Object].' + ); + component = ReactTestUtils.renderIntoDocument(); expect(() => component.forceUpdate(new Foo())).toThrowError( - 'forceUpdate(...): Expected the last optional `callback` argument ' + - 'to be a function. Instead received: Foo (keys: a, b).' + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: [object Object]', ); + expectDev(console.error.calls.argsFor(2)[0]).toContain( + 'forceUpdate(...): Expected the last optional `callback` argument to be ' + + 'a function. Instead received: [object Object].' + ); + expect(console.error.calls.count()).toBe(3); }); it('does not update one component twice in a batch (#2410)', () => { diff --git a/src/renderers/dom/shared/__tests__/ReactDOM-test.js b/src/renderers/dom/shared/__tests__/ReactDOM-test.js index 25dd09ec83..c7c574a8fb 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOM-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOM-test.js @@ -118,6 +118,8 @@ describe('ReactDOM', () => { }); it('throws in render() if the mount callback is not a function', () => { + spyOn(console, 'error'); + function Foo() { this.a = 1; this.b = 2; @@ -133,20 +135,35 @@ describe('ReactDOM', () => { var myDiv = document.createElement('div'); expect(() => ReactDOM.render(, myDiv, 'no')).toThrowError( - 'render(...): Expected the last optional `callback` argument ' + - 'to be a function. Instead received: string.' + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: no', ); - expect(() => ReactDOM.render(, myDiv, {})).toThrowError( - 'render(...): Expected the last optional `callback` argument ' + - 'to be a function. Instead received: Object.' + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'render(...): Expected the last optional `callback` argument to be ' + + 'a function. Instead received: no.' + ); + expect(() => ReactDOM.render(, myDiv, {foo: 'bar'})).toThrowError( + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: [object Object]', + ); + expectDev(console.error.calls.argsFor(1)[0]).toContain( + 'render(...): Expected the last optional `callback` argument to be ' + + 'a function. Instead received: [object Object].' ); expect(() => ReactDOM.render(, myDiv, new Foo())).toThrowError( - 'render(...): Expected the last optional `callback` argument ' + - 'to be a function. Instead received: Foo (keys: a, b).' + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: [object Object]', ); + expectDev(console.error.calls.argsFor(2)[0]).toContain( + 'render(...): Expected the last optional `callback` argument to be ' + + 'a function. Instead received: [object Object].' + ); + expect(console.error.calls.count()).toBe(3); }); it('throws in render() if the update callback is not a function', () => { + spyOn(console, 'error'); + function Foo() { this.a = 1; this.b = 2; @@ -162,19 +179,33 @@ describe('ReactDOM', () => { var myDiv = document.createElement('div'); ReactDOM.render(, myDiv); - expect(() => ReactDOM.render(, myDiv, 'no')).toThrowError( - 'render(...): Expected the last optional `callback` argument ' + - 'to be a function. Instead received: string.' + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: no', ); - expect(() => ReactDOM.render(, myDiv, {})).toThrowError( - 'render(...): Expected the last optional `callback` argument ' + - 'to be a function. Instead received: Object.' + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'render(...): Expected the last optional `callback` argument to be ' + + 'a function. Instead received: no.' ); + ReactDOM.render(, myDiv); // Re-mount + expect(() => ReactDOM.render(, myDiv, {foo: 'bar'})).toThrowError( + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: [object Object]', + ); + expectDev(console.error.calls.argsFor(1)[0]).toContain( + 'render(...): Expected the last optional `callback` argument to be ' + + 'a function. Instead received: [object Object].' + ); + ReactDOM.render(, myDiv); // Re-mount expect(() => ReactDOM.render(, myDiv, new Foo())).toThrowError( - 'render(...): Expected the last optional `callback` argument ' + - 'to be a function. Instead received: Foo (keys: a, b).' + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: [object Object]', ); + expectDev(console.error.calls.argsFor(2)[0]).toContain( + 'render(...): Expected the last optional `callback` argument to be ' + + 'a function. Instead received: [object Object].' + ); + expect(console.error.calls.count()).toBe(3); }); it('preserves focus', () => { diff --git a/src/renderers/dom/stack/client/ReactMount.js b/src/renderers/dom/stack/client/ReactMount.js index cd3fe68503..a1554d81bd 100644 --- a/src/renderers/dom/stack/client/ReactMount.js +++ b/src/renderers/dom/stack/client/ReactMount.js @@ -384,6 +384,7 @@ var ReactMount = { if (callback) { componentInstance._pendingCallbacks = [function() { + validateCallback(callback); callback.call(componentInstance._renderedComponent.getPublicInstance()); }]; } @@ -433,7 +434,15 @@ var ReactMount = { }, _renderSubtreeIntoContainer: function(parentComponent, nextElement, container, callback) { - validateCallback(callback, 'ReactDOM.render'); + callback = callback === undefined ? null : callback; + if (__DEV__) { + warning( + callback === null || typeof callback === 'function', + 'render(...): Expected the last optional `callback` argument to be a ' + + 'function. Instead received: %s.', + String(callback) + ); + } if (!React.isValidElement(nextElement)) { if (typeof nextElement === 'string') { invariant( @@ -489,6 +498,7 @@ var ReactMount = { if (shouldUpdateReactComponent(prevElement, nextElement)) { var publicInst = prevComponent._renderedComponent.getPublicInstance(); var updatedCallback = callback && function() { + validateCallback(callback); callback.call(publicInst); }; ReactMount._updateRootComponent( diff --git a/src/renderers/shared/fiber/ReactFiberClassComponent.js b/src/renderers/shared/fiber/ReactFiberClassComponent.js index 94fbf67c21..87f3426099 100644 --- a/src/renderers/shared/fiber/ReactFiberClassComponent.js +++ b/src/renderers/shared/fiber/ReactFiberClassComponent.js @@ -35,11 +35,23 @@ var { getComponentName, isMounted } = require('ReactFiberTreeReflection'); var ReactInstanceMap = require('ReactInstanceMap'); var emptyObject = require('emptyObject'); var shallowEqual = require('shallowEqual'); -var warning = require('warning'); var invariant = require('invariant'); const isArray = Array.isArray; +if (__DEV__) { + var warning = require('warning'); + var warnOnInvalidCallback = function(callback : mixed, callerName : string) { + warning( + callback === null || typeof callback === 'function', + '%s(...): Expected the last optional `callback` argument to be a ' + + 'function. Instead received: %s.', + callerName, + String(callback) + ); + }; +} + module.exports = function( scheduleUpdate : (fiber : Fiber, priorityLevel : PriorityLevel) => void, getPriorityContext : () => PriorityLevel, @@ -53,19 +65,31 @@ module.exports = function( enqueueSetState(instance, partialState, callback) { const fiber = ReactInstanceMap.get(instance); const priorityLevel = getPriorityContext(); - addUpdate(fiber, partialState, callback || null, priorityLevel); + callback = callback === undefined ? null : callback; + if (__DEV__) { + warnOnInvalidCallback(callback, 'setState'); + } + addUpdate(fiber, partialState, callback, priorityLevel); scheduleUpdate(fiber, priorityLevel); }, enqueueReplaceState(instance, state, callback) { const fiber = ReactInstanceMap.get(instance); const priorityLevel = getPriorityContext(); - addReplaceUpdate(fiber, state, callback || null, priorityLevel); + callback = callback === undefined ? null : callback; + if (__DEV__) { + warnOnInvalidCallback(callback, 'replaceState'); + } + addReplaceUpdate(fiber, state, callback, priorityLevel); scheduleUpdate(fiber, priorityLevel); }, enqueueForceUpdate(instance, callback) { const fiber = ReactInstanceMap.get(instance); const priorityLevel = getPriorityContext(); - addForceUpdate(fiber, callback || null, priorityLevel); + callback = callback === undefined ? null : callback; + if (__DEV__) { + warnOnInvalidCallback(callback, 'forceUpdate'); + } + addForceUpdate(fiber, callback, priorityLevel); scheduleUpdate(fiber, priorityLevel); }, }; diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index 1a429daf0a..d42b582128 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -30,6 +30,7 @@ var { createFiberRoot } = require('ReactFiberRoot'); var ReactFiberScheduler = require('ReactFiberScheduler'); if (__DEV__) { + var warning = require('warning'); var ReactFiberInstrumentation = require('ReactFiberInstrumentation'); } @@ -145,6 +146,15 @@ module.exports = function( function scheduleTopLevelUpdate(current : Fiber, element : ReactNodeList, callback : ?Function) { const priorityLevel = getPriorityContext(); const nextState = { element }; + callback = callback === undefined ? null : callback; + if (__DEV__) { + warning( + callback === null || typeof callback === 'function', + 'render(...): Expected the last optional `callback` argument to be a ' + + 'function. Instead received: %s.', + String(callback) + ); + } addTopLevelUpdate(current, nextState, callback || null, priorityLevel); scheduleUpdate(current, priorityLevel); } diff --git a/src/renderers/shared/fiber/ReactFiberUpdateQueue.js b/src/renderers/shared/fiber/ReactFiberUpdateQueue.js index bb70bec77d..8bb1d1401d 100644 --- a/src/renderers/shared/fiber/ReactFiberUpdateQueue.js +++ b/src/renderers/shared/fiber/ReactFiberUpdateQueue.js @@ -25,14 +25,17 @@ const { TaskPriority, } = require('ReactPriorityLevel'); -const validateCallback = require('validateCallback'); -const warning = require('warning'); +const invariant = require('invariant'); +if (__DEV__) { + var warning = require('warning'); +} type PartialState = $Subtype | (prevState: State, props: Props) => $Subtype; -type Callback = () => void; +// Callbacks are not validated until invocation +type Callback = mixed; type Update = { priorityLevel: PriorityLevel, @@ -215,8 +218,6 @@ function findInsertionPosition(queue, update) : Update | null { // // If the update is cloned, it returns the cloned update. 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; @@ -287,7 +288,7 @@ function insertUpdate(fiber : Fiber, update : Update, methodName : string) : Upd function addUpdate( fiber : Fiber, partialState : PartialState | null, - callback : Callback | null, + callback : mixed, priorityLevel : PriorityLevel ) : void { const update = { @@ -446,7 +447,7 @@ function beginUpdateQueue( } // Second condition ignores top-level unmount callbacks if they are not the // last update in the queue, since a subsequent update will cause a remount. - if (update.callback && !(update.isTopLevelUnmount && update.next)) { + if (update.callback !== null && !(update.isTopLevelUnmount && update.next !== null)) { callbackList = callbackList || []; callbackList.push(update.callback); workInProgress.effectTag |= CallbackEffect; @@ -476,6 +477,12 @@ function commitCallbacks(finishedWork : Fiber, queue : UpdateQueue, context : mi } for (let i = 0; i < callbackList.length; i++) { const callback = callbackList[i]; + invariant( + typeof callback === 'function', + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: %s', + String(callback) + ); callback.call(context); } } diff --git a/src/renderers/shared/stack/reconciler/CallbackQueue.js b/src/renderers/shared/stack/reconciler/CallbackQueue.js index 84e87cac3a..a6cd976129 100644 --- a/src/renderers/shared/stack/reconciler/CallbackQueue.js +++ b/src/renderers/shared/stack/reconciler/CallbackQueue.js @@ -15,6 +15,7 @@ var PooledClass = require('PooledClass'); var invariant = require('invariant'); +var validateCallback = require('validateCallback'); /** * A specialized pseudo-event module to help keep track of components waiting to @@ -70,6 +71,7 @@ class CallbackQueue { this._callbacks = null; this._contexts = null; for (var i = 0; i < callbacks.length; i++) { + validateCallback(callbacks[i]); callbacks[i].call(contexts[i], arg); } callbacks.length = 0; diff --git a/src/renderers/shared/stack/reconciler/ReactUpdateQueue.js b/src/renderers/shared/stack/reconciler/ReactUpdateQueue.js index a9bed9832c..3fbdf6c741 100644 --- a/src/renderers/shared/stack/reconciler/ReactUpdateQueue.js +++ b/src/renderers/shared/stack/reconciler/ReactUpdateQueue.js @@ -16,8 +16,18 @@ var ReactInstanceMap = require('ReactInstanceMap'); var ReactInstrumentation = require('ReactInstrumentation'); var ReactUpdates = require('ReactUpdates'); -var warning = require('warning'); -var validateCallback = require('validateCallback'); +if (__DEV__) { + var warning = require('warning'); + var warnOnInvalidCallback = function(callback : mixed, callerName : string) { + warning( + callback === null || typeof callback === 'function', + '%s(...): Expected the last optional `callback` argument to be a ' + + 'function. Instead received: %s.', + callerName, + String(callback) + ); + }; +} function enqueueUpdate(internalInstance) { ReactUpdates.enqueueUpdate(internalInstance); @@ -134,7 +144,8 @@ var ReactUpdateQueue = { } if (callback) { - validateCallback(callback, callerName); + callback = callback === undefined ? null : callback; + warnOnInvalidCallback(callback, callerName); if (internalInstance._pendingCallbacks) { internalInstance._pendingCallbacks.push(callback); } else { @@ -174,7 +185,8 @@ var ReactUpdateQueue = { internalInstance._pendingReplaceState = true; if (callback) { - validateCallback(callback, callerName); + callback = callback === undefined ? null : callback; + warnOnInvalidCallback(callback, callerName); if (internalInstance._pendingCallbacks) { internalInstance._pendingCallbacks.push(callback); } else { @@ -222,7 +234,8 @@ var ReactUpdateQueue = { queue.push(partialState); if (callback) { - validateCallback(callback, callerName); + callback = callback === undefined ? null : callback; + warnOnInvalidCallback(callback, callerName); if (internalInstance._pendingCallbacks) { internalInstance._pendingCallbacks.push(callback); } else { diff --git a/src/renderers/shared/utils/validateCallback.js b/src/renderers/shared/utils/validateCallback.js index 65c0260db9..657265b84f 100644 --- a/src/renderers/shared/utils/validateCallback.js +++ b/src/renderers/shared/utils/validateCallback.js @@ -14,33 +14,13 @@ 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) { - if ( - callback !== null && - callback !== undefined && - typeof callback !== 'function' - ) { - invariant( - false, - '%s(...): Expected the last optional `callback` argument to be a ' + - 'function. Instead received: %s.', - callerName, - formatUnexpectedArgument(callback) - ); - } +function validateCallback(callback: ?Function) { + invariant( + !callback || typeof callback === 'function', + 'Invalid argument passed as callback. Expected a function. Instead ' + + 'received: %s', + String(callback) + ); } module.exports = validateCallback;