Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more specific error messages for bad callback in setState, replaceState, and ReactDOM.render #6310

Merged
merged 4 commits into from
Mar 23, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/isomorphic/classic/class/ReactClass.js
Original file line number Diff line number Diff line change
Expand Up @@ -710,7 +710,7 @@ var ReactClassMixin = {
replaceState: function(newState, callback) {
this.updater.enqueueReplaceState(this, newState);
if (callback) {
this.updater.enqueueCallback(this, callback);
this.updater.enqueueCallback(this, callback, 'replaceState');
}
},

Expand Down
4 changes: 2 additions & 2 deletions src/isomorphic/modern/class/ReactComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ ReactComponent.prototype.setState = function(partialState, callback) {
}
this.updater.enqueueSetState(this, partialState);
if (callback) {
this.updater.enqueueCallback(this, callback);
this.updater.enqueueCallback(this, callback, 'setState');
}
};

Expand All @@ -97,7 +97,7 @@ ReactComponent.prototype.setState = function(partialState, callback) {
ReactComponent.prototype.forceUpdate = function(callback) {
this.updater.enqueueForceUpdate(this);
if (callback) {
this.updater.enqueueCallback(this, callback);
this.updater.enqueueCallback(this, callback, 'forceUpdate');
}
};

Expand Down
1 change: 1 addition & 0 deletions src/renderers/dom/client/ReactMount.js
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,7 @@ var ReactMount = {
},

_renderSubtreeIntoContainer: function(parentComponent, nextElement, container, callback) {
ReactUpdateQueue.validateCallback(callback, 'ReactDOM.render');
invariant(
ReactElement.isValidElement(nextElement),
'ReactDOM.render(): Invalid component element.%s',
Expand Down
59 changes: 59 additions & 0 deletions src/renderers/dom/client/__tests__/ReactDOM-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,4 +118,63 @@ describe('ReactDOM', function() {
expect(console.error.argsForCall.length).toBe(0);
});

it('throws in render() if the mount callback is not a function', function() {
function Foo() {
this.a = 1;
this.b = 2;
}
var A = React.createClass({
getInitialState: function() {
return {};
},
render: function() {
return <div />;
},
});

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

it('throws in render() if the update callback is not a function', function() {
function Foo() {
this.a = 1;
this.b = 2;
}
var A = React.createClass({
getInitialState: function() {
return {};
},
render: function() {
return <div />;
},
});

var myDiv = document.createElement('div');
ReactDOM.render(<A />, myDiv);

expect(() => ReactDOM.render(<A />, myDiv, 'no')).toThrow(
'ReactDOM.render(...): Expected the last optional `callback` argument ' +
'to be a function. Instead received: string.'
);
expect(() => ReactDOM.render(<A />, myDiv, {})).toThrow(
'ReactDOM.render(...): Expected the last optional `callback` argument ' +
'to be a function. Instead received: Object.'
);
expect(() => ReactDOM.render(<A />, myDiv, new Foo())).toThrow(
'ReactDOM.render(...): Expected the last optional `callback` argument ' +
'to be a function. Instead received: Foo (keys: a, b).'
);
});
});
43 changes: 26 additions & 17 deletions src/renderers/shared/reconciler/ReactUpdateQueue.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,19 @@ 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) {
Expand Down Expand Up @@ -103,17 +116,11 @@ var ReactUpdateQueue = {
*
* @param {ReactClass} publicInstance The instance to use as `this` context.
* @param {?function} callback Called after state is updated.
* @param {string} callerName Name of the calling function in the public API.
* @internal
*/
enqueueCallback: function(publicInstance, callback) {
invariant(
typeof callback === 'function',
'enqueueCallback(...): You called `setProps`, `replaceProps`, ' +
'`setState`, `replaceState`, or `forceUpdate` with a callback of type ' +
'%s. A function is expected',
typeof callback === 'object' && Object.keys(callback).length && Object.keys(callback).length < 20 ?
typeof callback + ' (keys: ' + Object.keys(callback) + ')' : typeof callback
);
enqueueCallback: function(publicInstance, callback, callerName) {
ReactUpdateQueue.validateCallback(callback, callerName);
var internalInstance = getInternalInstanceReadyForUpdate(publicInstance);

// Previously we would throw an error if we didn't have an internal
Expand All @@ -138,14 +145,6 @@ var ReactUpdateQueue = {
},

enqueueCallbackInternal: function(internalInstance, callback) {
invariant(
typeof callback === 'function',
'enqueueCallback(...): You called `setProps`, `replaceProps`, ' +
'`setState`, `replaceState`, or `forceUpdate` with a callback of type ' +
'%s. A function is expected',
typeof callback === 'object' && Object.keys(callback).length && Object.keys(callback).length < 20 ?
typeof callback + ' (keys: ' + Object.keys(callback) + ')' : typeof callback
);
if (internalInstance._pendingCallbacks) {
internalInstance._pendingCallbacks.push(callback);
} else {
Expand Down Expand Up @@ -242,6 +241,16 @@ 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;
87 changes: 87 additions & 0 deletions src/renderers/shared/reconciler/__tests__/ReactUpdates-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -937,4 +937,91 @@ describe('ReactUpdates', function() {
ReactFeatureFlags.logTopLevelRenders = false;
}
});

it('throws in setState if the update callback is not a function', function() {
function Foo() {
this.a = 1;
this.b = 2;
}
var A = React.createClass({
getInitialState: function() {
return {};
},
render: function() {
return <div />;
},
});
var component = ReactTestUtils.renderIntoDocument(<A />);

expect(() => component.setState({}, 'no')).toThrow(
'setState(...): Expected the last optional `callback` argument ' +
'to be a function. Instead received: string.'
);
expect(() => component.setState({}, {})).toThrow(
'setState(...): Expected the last optional `callback` argument ' +
'to be a function. Instead received: Object.'
);
expect(() => component.setState({}, new Foo())).toThrow(
'setState(...): Expected the last optional `callback` argument ' +
'to be a function. Instead received: Foo (keys: a, b).'
);
});

it('throws in replaceState if the update callback is not a function', function() {
function Foo() {
this.a = 1;
this.b = 2;
}
var A = React.createClass({
getInitialState: function() {
return {};
},
render: function() {
return <div />;
},
});
var component = ReactTestUtils.renderIntoDocument(<A />);

expect(() => component.replaceState({}, 'no')).toThrow(
'replaceState(...): Expected the last optional `callback` argument ' +
'to be a function. Instead received: string.'
);
expect(() => component.replaceState({}, {})).toThrow(
'replaceState(...): Expected the last optional `callback` argument ' +
'to be a function. Instead received: Object.'
);
expect(() => component.replaceState({}, new Foo())).toThrow(
'replaceState(...): Expected the last optional `callback` argument ' +
'to be a function. Instead received: Foo (keys: a, b).'
);
});

it('throws in forceUpdate if the update callback is not a function', function() {
function Foo() {
this.a = 1;
this.b = 2;
}
var A = React.createClass({
getInitialState: function() {
return {};
},
render: function() {
return <div />;
},
});
var component = ReactTestUtils.renderIntoDocument(<A />);

expect(() => component.forceUpdate('no')).toThrow(
'forceUpdate(...): Expected the last optional `callback` argument ' +
'to be a function. Instead received: string.'
);
expect(() => component.forceUpdate({})).toThrow(
'forceUpdate(...): Expected the last optional `callback` argument ' +
'to be a function. Instead received: Object.'
);
expect(() => component.forceUpdate(new Foo())).toThrow(
'forceUpdate(...): Expected the last optional `callback` argument ' +
'to be a function. Instead received: Foo (keys: a, b).'
);
});
});