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

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Mar 21, 2016

This builds on #5193 and introduces an important, in my opinion, detail (constructor function name) that makes issues like #6306 more easily identified.

Additionally, this removes the mention of methods that have been removed (setProps and replaceProps).

When using onClick={this.setState.bind(this, nextState)} by mistake, the error message says:

0.14

Uncaught Invariant Violation: enqueueCallback(...): You called setProps, replaceProps, setState, replaceState, or forceUpdate with a callback that isn't callable.

After #5193

Uncaught Invariant Violation: enqueueCallback(...): You called setProps, replaceProps, setState, replaceState, or forceUpdate with a callback of type object. A function is expected

After this PR

Uncaught Invariant Violation: setState(...): Expected the last optional callback argument to be a function. Instead received: SyntheticMouseEvent.


Why?

  • We don’t need to mention the APIs we removed (setProps, replaceProps).
  • enqueueCallback() is an implementation detail and is confusing to the user.
  • We have enough information to print the actual public API method name.
  • We need to make it clear what “callback” is. Many people don’t know these functions accept a callback. We need to tell them it’s the last optional argument.
  • Printing the constructor name (e.g. SyntheticMouseEvent) makes it much more clear why you get issues like 'Bind' will result in errors in onClick handler #6306.
  • This error should also be enabled for ReactDOM.render() instead of just failing with a TypeError.
  • This adds tests.

@gaearon
Copy link
Collaborator Author

gaearon commented Mar 21, 2016

I also noticed that top-level render does not have a similar protection and just fails with Uncaught TypeError: callback.call is not a function. I can look into fixing this as well, but at this point I would rather add tests so we don’t regress.

@facebook-github-bot
Copy link

@gaearon updated the pull request.

@facebook-github-bot
Copy link

@gaearon updated the pull request.

@facebook-github-bot
Copy link

@gaearon updated the pull request.

@gaearon
Copy link
Collaborator Author

gaearon commented Mar 21, 2016

OK, this is ready for review. I changed error messages to include the specific method name and also added a descriptive error to ReactDOM.render failures.

cc @jimfb @zpao

@gaearon gaearon changed the title Make setState() callback error message more descriptive Add more specific error messages for bad callback in setState, replaceState, and ReactDOM.render Mar 21, 2016
@facebook-github-bot
Copy link

@gaearon updated the pull request.

@@ -138,14 +145,6 @@ var ReactUpdateQueue = {
},

enqueueCallbackInternal: function(internalInstance, callback) {
invariant(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this one because it’s not enough for ReactDOM.render() code path. ReactMount calls the callback synchronously on the initial render so we would fail to print the error if we only checked here.

Currently ReactMount is the only thing using enqueueCallbackInternal. I could put an additional validateCallback call here too but then it would run twice on every initial render.

@jimfb
Copy link
Contributor

jimfb commented Mar 22, 2016

👍

@gaearon gaearon merged commit 1e81561 into facebook:master Mar 23, 2016
@gaearon gaearon deleted the setstate-warning branch March 23, 2016 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants