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.
Andrew Clark committed
Feb 9, 2017 at 15:35 UTC
d3aeca1ff89e2ecedcd9be5ebe0a08336def5048
9 files changed
+208
-80
src/renderers/__tests__/ReactUpdates-test.js
+72
-21
@@ -901,6 +901,8 @@ describe('ReactUpdates', () => {
901
});
902
903
it('throws in setState if the update callback is not a function', () => {
904
+ spyOn(console, 'error');
905
+
906
function Foo() {
907
this.a = 1;
908
this.b = 2;
@@ -917,20 +919,37 @@ describe('ReactUpdates', () => {
919
var component = ReactTestUtils.renderIntoDocument(<A />);
920
921
expect(() => component.setState({}, 'no')).toThrowError(
920
- 'setState(...): Expected the last optional `callback` argument ' +
921
- 'to be a function. Instead received: string.'
922
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
923
+ 'received: no',
924
+ );
925
+ expectDev(console.error.calls.argsFor(0)[0]).toContain(
926
+ 'setState(...): Expected the last optional `callback` argument to be ' +
927
+ 'a function. Instead received: no.'
928
+ );
929
+ component = ReactTestUtils.renderIntoDocument(<A />);
930
+ expect(() => component.setState({}, {foo: 'bar'})).toThrowError(
931
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
932
+ 'received: [object Object]',
933
);
923
- expect(() => component.setState({}, {})).toThrowError(
924
- 'setState(...): Expected the last optional `callback` argument ' +
925
- 'to be a function. Instead received: Object.'
934
+ expectDev(console.error.calls.argsFor(1)[0]).toContain(
935
+ 'setState(...): Expected the last optional `callback` argument to be ' +
936
+ 'a function. Instead received: [object Object].'
937
);
938
+ component = ReactTestUtils.renderIntoDocument(<A />);
939
expect(() => component.setState({}, new Foo())).toThrowError(
928
- 'setState(...): Expected the last optional `callback` argument ' +
929
- 'to be a function. Instead received: Foo (keys: a, b).'
940
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
941
+ 'received: [object Object]',
942
);
943
+ expectDev(console.error.calls.argsFor(2)[0]).toContain(
944
+ 'setState(...): Expected the last optional `callback` argument to be ' +
945
+ 'a function. Instead received: [object Object].'
946
+ );
947
+ expect(console.error.calls.count()).toBe(3);
948
});
949
950
it('throws in replaceState if the update callback is not a function', () => {
951
+ spyOn(console, 'error');
952
+
953
function Foo() {
954
this.a = 1;
955
this.b = 2;
@@ -946,20 +965,37 @@ describe('ReactUpdates', () => {
965
var component = ReactTestUtils.renderIntoDocument(<A />);
966
967
expect(() => component.replaceState({}, 'no')).toThrowError(
949
- 'replaceState(...): Expected the last optional `callback` argument ' +
950
- 'to be a function. Instead received: string.'
968
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
969
+ 'received: no',
970
+ );
971
+ expectDev(console.error.calls.argsFor(0)[0]).toContain(
972
+ 'replaceState(...): Expected the last optional `callback` argument to be ' +
973
+ 'a function. Instead received: no.'
974
);
952
- expect(() => component.replaceState({}, {})).toThrowError(
953
- 'replaceState(...): Expected the last optional `callback` argument ' +
954
- 'to be a function. Instead received: Object.'
975
+ component = ReactTestUtils.renderIntoDocument(<A />);
976
+ expect(() => component.replaceState({}, {foo: 'bar'})).toThrowError(
977
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
978
+ 'received: [object Object]',
979
);
980
+ expectDev(console.error.calls.argsFor(1)[0]).toContain(
981
+ 'replaceState(...): Expected the last optional `callback` argument to be ' +
982
+ 'a function. Instead received: [object Object].'
983
+ );
984
+ component = ReactTestUtils.renderIntoDocument(<A />);
985
expect(() => component.replaceState({}, new Foo())).toThrowError(
957
- 'replaceState(...): Expected the last optional `callback` argument ' +
958
- 'to be a function. Instead received: Foo (keys: a, b).'
986
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
987
+ 'received: [object Object]',
988
+ );
989
+ expectDev(console.error.calls.argsFor(2)[0]).toContain(
990
+ 'replaceState(...): Expected the last optional `callback` argument to be ' +
991
+ 'a function. Instead received: [object Object].'
992
);
993
+ expect(console.error.calls.count()).toBe(3);
994
});
995
996
it('throws in forceUpdate if the update callback is not a function', () => {
997
+ spyOn(console, 'error');
998
+
999
function Foo() {
1000
this.a = 1;
1001
this.b = 2;
@@ -976,17 +1012,32 @@ describe('ReactUpdates', () => {
1012
var component = ReactTestUtils.renderIntoDocument(<A />);
1013
1014
expect(() => component.forceUpdate('no')).toThrowError(
979
- 'forceUpdate(...): Expected the last optional `callback` argument ' +
980
- 'to be a function. Instead received: string.'
1015
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
1016
+ 'received: no',
1017
);
982
- expect(() => component.forceUpdate({})).toThrowError(
983
- 'forceUpdate(...): Expected the last optional `callback` argument ' +
984
- 'to be a function. Instead received: Object.'
1018
+ expectDev(console.error.calls.argsFor(0)[0]).toContain(
1019
+ 'forceUpdate(...): Expected the last optional `callback` argument to be ' +
1020
+ 'a function. Instead received: no.'
1021
);
1022
+ component = ReactTestUtils.renderIntoDocument(<A />);
1023
+ expect(() => component.forceUpdate({foo: 'bar'})).toThrowError(
1024
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
1025
+ 'received: [object Object]',
1026
+ );
1027
+ expectDev(console.error.calls.argsFor(1)[0]).toContain(
1028
+ 'forceUpdate(...): Expected the last optional `callback` argument to be ' +
1029
+ 'a function. Instead received: [object Object].'
1030
+ );
1031
+ component = ReactTestUtils.renderIntoDocument(<A />);
1032
expect(() => component.forceUpdate(new Foo())).toThrowError(
987
- 'forceUpdate(...): Expected the last optional `callback` argument ' +
988
- 'to be a function. Instead received: Foo (keys: a, b).'
1033
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
1034
+ 'received: [object Object]',
1035
+ );
1036
+ expectDev(console.error.calls.argsFor(2)[0]).toContain(
1037
+ 'forceUpdate(...): Expected the last optional `callback` argument to be ' +
1038
+ 'a function. Instead received: [object Object].'
1039
);
1040
+ expect(console.error.calls.count()).toBe(3);
1041
});
1042
1043
it('does not update one component twice in a batch (#2410)', () => {
src/renderers/dom/shared/__tests__/ReactDOM-test.js
+46
-15
@@ -118,6 +118,8 @@ describe('ReactDOM', () => {
118
});
119
120
it('throws in render() if the mount callback is not a function', () => {
121
+ spyOn(console, 'error');
122
+
123
function Foo() {
124
this.a = 1;
125
this.b = 2;
@@ -133,20 +135,35 @@ describe('ReactDOM', () => {
135
136
var myDiv = document.createElement('div');
137
expect(() => ReactDOM.render(<A />, myDiv, 'no')).toThrowError(
136
- 'render(...): Expected the last optional `callback` argument ' +
137
- 'to be a function. Instead received: string.'
138
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
139
+ 'received: no',
140
+ );
141
+ expectDev(console.error.calls.argsFor(0)[0]).toContain(
142
+ 'render(...): Expected the last optional `callback` argument to be ' +
143
+ 'a function. Instead received: no.'
144
);
139
- expect(() => ReactDOM.render(<A />, myDiv, {})).toThrowError(
140
- 'render(...): Expected the last optional `callback` argument ' +
141
- 'to be a function. Instead received: Object.'
145
+ expect(() => ReactDOM.render(<A />, myDiv, {foo: 'bar'})).toThrowError(
146
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
147
+ 'received: [object Object]',
148
+ );
149
+ expectDev(console.error.calls.argsFor(1)[0]).toContain(
150
+ 'render(...): Expected the last optional `callback` argument to be ' +
151
+ 'a function. Instead received: [object Object].'
152
);
153
expect(() => ReactDOM.render(<A />, myDiv, new Foo())).toThrowError(
144
- 'render(...): Expected the last optional `callback` argument ' +
145
- 'to be a function. Instead received: Foo (keys: a, b).'
154
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
155
+ 'received: [object Object]',
156
+ );
157
+ expectDev(console.error.calls.argsFor(2)[0]).toContain(
158
+ 'render(...): Expected the last optional `callback` argument to be ' +
159
+ 'a function. Instead received: [object Object].'
160
);
161
+ expect(console.error.calls.count()).toBe(3);
162
});
163
164
it('throws in render() if the update callback is not a function', () => {
165
+ spyOn(console, 'error');
166
+
167
function Foo() {
168
this.a = 1;
169
this.b = 2;
@@ -162,19 +179,33 @@ describe('ReactDOM', () => {
179
180
var myDiv = document.createElement('div');
181
ReactDOM.render(<A />, myDiv);
165
-
182
expect(() => ReactDOM.render(<A />, myDiv, 'no')).toThrowError(
167
- 'render(...): Expected the last optional `callback` argument ' +
168
- 'to be a function. Instead received: string.'
183
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
184
+ 'received: no',
185
+ );
186
+ expectDev(console.error.calls.argsFor(0)[0]).toContain(
187
+ 'render(...): Expected the last optional `callback` argument to be ' +
188
+ 'a function. Instead received: no.'
189
);
170
- expect(() => ReactDOM.render(<A />, myDiv, {})).toThrowError(
171
- 'render(...): Expected the last optional `callback` argument ' +
172
- 'to be a function. Instead received: Object.'
190
+ ReactDOM.render(<A />, myDiv); // Re-mount
191
+ expect(() => ReactDOM.render(<A />, myDiv, {foo: 'bar'})).toThrowError(
192
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
193
+ 'received: [object Object]',
194
);
195
+ expectDev(console.error.calls.argsFor(1)[0]).toContain(
196
+ 'render(...): Expected the last optional `callback` argument to be ' +
197
+ 'a function. Instead received: [object Object].'
198
+ );
199
+ ReactDOM.render(<A />, myDiv); // Re-mount
200
expect(() => ReactDOM.render(<A />, myDiv, new Foo())).toThrowError(
175
- 'render(...): Expected the last optional `callback` argument ' +
176
- 'to be a function. Instead received: Foo (keys: a, b).'
201
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
202
+ 'received: [object Object]',
203
+ );
204
+ expectDev(console.error.calls.argsFor(2)[0]).toContain(
205
+ 'render(...): Expected the last optional `callback` argument to be ' +
206
+ 'a function. Instead received: [object Object].'
207
);
208
+ expect(console.error.calls.count()).toBe(3);
209
});
210
211
it('preserves focus', () => {
src/renderers/dom/stack/client/ReactMount.js
+11
-1
@@ -384,6 +384,7 @@ var ReactMount = {
384
385
if (callback) {
386
componentInstance._pendingCallbacks = [function() {
387
+ validateCallback(callback);
388
callback.call(componentInstance._renderedComponent.getPublicInstance());
389
}];
390
}
@@ -433,7 +434,15 @@ var ReactMount = {
434
},
435
436
_renderSubtreeIntoContainer: function(parentComponent, nextElement, container, callback) {
436
- validateCallback(callback, 'ReactDOM.render');
437
+ callback = callback === undefined ? null : callback;
438
+ if (__DEV__) {
439
+ warning(
440
+ callback === null || typeof callback === 'function',
441
+ 'render(...): Expected the last optional `callback` argument to be a ' +
442
+ 'function. Instead received: %s.',
443
+ String(callback)
444
+ );
445
+ }
446
if (!React.isValidElement(nextElement)) {
447
if (typeof nextElement === 'string') {
448
invariant(
@@ -489,6 +498,7 @@ var ReactMount = {
498
if (shouldUpdateReactComponent(prevElement, nextElement)) {
499
var publicInst = prevComponent._renderedComponent.getPublicInstance();
500
var updatedCallback = callback && function() {
501
+ validateCallback(callback);
502
callback.call(publicInst);
503
};
504
ReactMount._updateRootComponent(
src/renderers/shared/fiber/ReactFiberClassComponent.js
+28
-4
@@ -35,11 +35,23 @@ var { getComponentName, isMounted } = require('ReactFiberTreeReflection');
35
var ReactInstanceMap = require('ReactInstanceMap');
36
var emptyObject = require('emptyObject');
37
var shallowEqual = require('shallowEqual');
38
-var warning = require('warning');
38
var invariant = require('invariant');
39
40
const isArray = Array.isArray;
41
42
+if (__DEV__) {
43
+ var warning = require('warning');
44
+ var warnOnInvalidCallback = function(callback : mixed, callerName : string) {
45
+ warning(
46
+ callback === null || typeof callback === 'function',
47
+ '%s(...): Expected the last optional `callback` argument to be a ' +
48
+ 'function. Instead received: %s.',
49
+ callerName,
50
+ String(callback)
51
+ );
52
+ };
53
+}
54
+
55
module.exports = function(
56
scheduleUpdate : (fiber : Fiber, priorityLevel : PriorityLevel) => void,
57
getPriorityContext : () => PriorityLevel,
@@ -53,19 +65,31 @@ module.exports = function(
65
enqueueSetState(instance, partialState, callback) {
66
const fiber = ReactInstanceMap.get(instance);
67
const priorityLevel = getPriorityContext();
56
- addUpdate(fiber, partialState, callback || null, priorityLevel);
68
+ callback = callback === undefined ? null : callback;
69
+ if (__DEV__) {
70
+ warnOnInvalidCallback(callback, 'setState');
71
+ }
72
+ addUpdate(fiber, partialState, callback, priorityLevel);
73
scheduleUpdate(fiber, priorityLevel);
74
},
75
enqueueReplaceState(instance, state, callback) {
76
const fiber = ReactInstanceMap.get(instance);
77
const priorityLevel = getPriorityContext();
62
- addReplaceUpdate(fiber, state, callback || null, priorityLevel);
78
+ callback = callback === undefined ? null : callback;
79
+ if (__DEV__) {
80
+ warnOnInvalidCallback(callback, 'replaceState');
81
+ }
82
+ addReplaceUpdate(fiber, state, callback, priorityLevel);
83
scheduleUpdate(fiber, priorityLevel);
84
},
85
enqueueForceUpdate(instance, callback) {
86
const fiber = ReactInstanceMap.get(instance);
87
const priorityLevel = getPriorityContext();
68
- addForceUpdate(fiber, callback || null, priorityLevel);
88
+ callback = callback === undefined ? null : callback;
89
+ if (__DEV__) {
90
+ warnOnInvalidCallback(callback, 'forceUpdate');
91
+ }
92
+ addForceUpdate(fiber, callback, priorityLevel);
93
scheduleUpdate(fiber, priorityLevel);
94
},
95
};
src/renderers/shared/fiber/ReactFiberReconciler.js
+10
@@ -30,6 +30,7 @@ var { createFiberRoot } = require('ReactFiberRoot');
30
var ReactFiberScheduler = require('ReactFiberScheduler');
31
32
if (__DEV__) {
33
+ var warning = require('warning');
34
var ReactFiberInstrumentation = require('ReactFiberInstrumentation');
35
}
36
@@ -145,6 +146,15 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
146
function scheduleTopLevelUpdate(current : Fiber, element : ReactNodeList, callback : ?Function) {
147
const priorityLevel = getPriorityContext();
148
const nextState = { element };
149
+ callback = callback === undefined ? null : callback;
150
+ if (__DEV__) {
151
+ warning(
152
+ callback === null || typeof callback === 'function',
153
+ 'render(...): Expected the last optional `callback` argument to be a ' +
154
+ 'function. Instead received: %s.',
155
+ String(callback)
156
+ );
157
+ }
158
addTopLevelUpdate(current, nextState, callback || null, priorityLevel);
159
scheduleUpdate(current, priorityLevel);
160
}
src/renderers/shared/fiber/ReactFiberUpdateQueue.js
+14
-7
@@ -25,14 +25,17 @@ const {
25
TaskPriority,
26
} = require('ReactPriorityLevel');
27
28
-const validateCallback = require('validateCallback');
29
-const warning = require('warning');
28
+const invariant = require('invariant');
29
+if (__DEV__) {
30
+ var warning = require('warning');
31
+}
32
33
type PartialState<State, Props> =
34
$Subtype<State> |
35
(prevState: State, props: Props) => $Subtype<State>;
36
35
-type Callback = () => void;
37
+// Callbacks are not validated until invocation
38
+type Callback = mixed;
39
40
type Update = {
41
priorityLevel: PriorityLevel,
@@ -215,8 +218,6 @@ function findInsertionPosition(queue, update) : Update | null {
218
//
219
// If the update is cloned, it returns the cloned update.
220
function insertUpdate(fiber : Fiber, update : Update, methodName : string) : Update | null {
218
- validateCallback(update.callback, methodName);
219
-
221
const queue1 = ensureUpdateQueue(fiber);
222
const queue2 = fiber.alternate ? ensureUpdateQueue(fiber.alternate) : null;
223
@@ -287,7 +288,7 @@ function insertUpdate(fiber : Fiber, update : Update, methodName : string) : Upd
288
function addUpdate(
289
fiber : Fiber,
290
partialState : PartialState<any, any> | null,
290
- callback : Callback | null,
291
+ callback : mixed,
292
priorityLevel : PriorityLevel
293
) : void {
294
const update = {
@@ -446,7 +447,7 @@ function beginUpdateQueue(
447
}
448
// Second condition ignores top-level unmount callbacks if they are not the
449
// last update in the queue, since a subsequent update will cause a remount.
449
- if (update.callback && !(update.isTopLevelUnmount && update.next)) {
450
+ if (update.callback !== null && !(update.isTopLevelUnmount && update.next !== null)) {
451
callbackList = callbackList || [];
452
callbackList.push(update.callback);
453
workInProgress.effectTag |= CallbackEffect;
@@ -476,6 +477,12 @@ function commitCallbacks(finishedWork : Fiber, queue : UpdateQueue, context : mi
477
}
478
for (let i = 0; i < callbackList.length; i++) {
479
const callback = callbackList[i];
480
+ invariant(
481
+ typeof callback === 'function',
482
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
483
+ 'received: %s',
484
+ String(callback)
485
+ );
486
callback.call(context);
487
}
488
}
src/renderers/shared/stack/reconciler/CallbackQueue.js
+2
@@ -15,6 +15,7 @@
15
var PooledClass = require('PooledClass');
16
17
var invariant = require('invariant');
18
+var validateCallback = require('validateCallback');
19
20
/**
21
* A specialized pseudo-event module to help keep track of components waiting to
@@ -70,6 +71,7 @@ class CallbackQueue<T> {
71
this._callbacks = null;
72
this._contexts = null;
73
for (var i = 0; i < callbacks.length; i++) {
74
+ validateCallback(callbacks[i]);
75
callbacks[i].call(contexts[i], arg);
76
}
77
callbacks.length = 0;
src/renderers/shared/stack/reconciler/ReactUpdateQueue.js
+18
-5
@@ -16,8 +16,18 @@ var ReactInstanceMap = require('ReactInstanceMap');
16
var ReactInstrumentation = require('ReactInstrumentation');
17
var ReactUpdates = require('ReactUpdates');
18
19
-var warning = require('warning');
20
-var validateCallback = require('validateCallback');
19
+if (__DEV__) {
20
+ var warning = require('warning');
21
+ var warnOnInvalidCallback = function(callback : mixed, callerName : string) {
22
+ warning(
23
+ callback === null || typeof callback === 'function',
24
+ '%s(...): Expected the last optional `callback` argument to be a ' +
25
+ 'function. Instead received: %s.',
26
+ callerName,
27
+ String(callback)
28
+ );
29
+ };
30
+}
31
32
function enqueueUpdate(internalInstance) {
33
ReactUpdates.enqueueUpdate(internalInstance);
@@ -134,7 +144,8 @@ var ReactUpdateQueue = {
144
}
145
146
if (callback) {
137
- validateCallback(callback, callerName);
147
+ callback = callback === undefined ? null : callback;
148
+ warnOnInvalidCallback(callback, callerName);
149
if (internalInstance._pendingCallbacks) {
150
internalInstance._pendingCallbacks.push(callback);
151
} else {
@@ -174,7 +185,8 @@ var ReactUpdateQueue = {
185
internalInstance._pendingReplaceState = true;
186
187
if (callback) {
177
- validateCallback(callback, callerName);
188
+ callback = callback === undefined ? null : callback;
189
+ warnOnInvalidCallback(callback, callerName);
190
if (internalInstance._pendingCallbacks) {
191
internalInstance._pendingCallbacks.push(callback);
192
} else {
@@ -222,7 +234,8 @@ var ReactUpdateQueue = {
234
queue.push(partialState);
235
236
if (callback) {
225
- validateCallback(callback, callerName);
237
+ callback = callback === undefined ? null : callback;
238
+ warnOnInvalidCallback(callback, callerName);
239
if (internalInstance._pendingCallbacks) {
240
internalInstance._pendingCallbacks.push(callback);
241
} else {
src/renderers/shared/utils/validateCallback.js
+7
-27
@@ -14,33 +14,13 @@
14
15
const invariant = require('invariant');
16
17
-function formatUnexpectedArgument(arg: any) {
18
- let type = typeof arg;
19
- if (type !== 'object') {
20
- return type;
21
- }
22
- let displayName = arg.constructor && arg.constructor.name || type;
23
- let keys = Object.keys(arg);
24
- if (keys.length > 0 && keys.length < 20) {
25
- return `${displayName} (keys: ${keys.join(', ')})`;
26
- }
27
- return displayName;
28
-}
29
-
30
-function validateCallback(callback: ?Function, callerName: string) {
31
- if (
32
- callback !== null &&
33
- callback !== undefined &&
34
- typeof callback !== 'function'
35
- ) {
36
- invariant(
37
- false,
38
- '%s(...): Expected the last optional `callback` argument to be a ' +
39
- 'function. Instead received: %s.',
40
- callerName,
41
- formatUnexpectedArgument(callback)
42
- );
43
- }
17
+function validateCallback(callback: ?Function) {
18
+ invariant(
19
+ !callback || typeof callback === 'function',
20
+ 'Invalid argument passed as callback. Expected a function. Instead ' +
21
+ 'received: %s',
22
+ String(callback)
23
+ );
24
}
25
26
module.exports = validateCallback;