Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Introduce much more detailed setState error messages #67

Merged
merged 2 commits into from
Apr 17, 2018

Conversation

LPGhatguy
Copy link
Contributor

@LPGhatguy LPGhatguy commented Apr 12, 2018

Fixes #66.

This PR replaces the single "you can't use setState here" message with a bunch of new messages, each tailored to the exact case.

They give much more useful information, and where possible, where to put your setState call instead.

It also catches an extra case where the render and reconciliation steps were not being guarded, specifically during reify.

@coveralls
Copy link

coveralls commented Apr 12, 2018

Coverage Status

Coverage increased (+0.3%) to 88.689% when pulling fcda293 on new-setstate-checks into 7f48175 on master.

Check the component %q to see if:
* setState could be called by a child Ref
* setState could be called by a child Changed event
* setState could be called by a child's render method]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, but might be clearer to say "setState is being called by...". Otherwise, it kind of sounds like a suggestion rather than a possible diagnosis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll probably also fold that part into the section before the list, like:

Check the component %q to see if setState is being called by:

  • a child Ref
  • a child Changed event
  • a child's render method

Check the definition of shouldUpdate in the component %q.]]

invalidSetStateMessages["init"] = [[
setState cannot be used in the init method.
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be helpful to mention that you can set state directly in init.

@LPGhatguy
Copy link
Contributor Author

All of the messages are updated.

Any other feedback?

@ZoteTheMighty
Copy link
Contributor

Nah, LGTM!

@AmaranthineCodices
Copy link
Contributor

LGTM as well

@LPGhatguy LPGhatguy merged commit 5baba17 into master Apr 17, 2018
@cliffchapmanrbx cliffchapmanrbx deleted the new-setstate-checks branch June 11, 2020 18:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants