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

setState sanity checking #23

Merged
merged 4 commits into from
Jan 29, 2018

Conversation

AmaranthineCodices
Copy link
Contributor

@AmaranthineCodices AmaranthineCodices commented Jan 29, 2018

This fixes #17. It throws an error when setState is called in the following locations:

  • the init function of a component
  • the render function of a component
  • any point after the component begins unmounting, including within willUnmount

The error message:

State cannot be set at this point: are you setting state from an init, render, or willUnmount function?

Tests are included, yay!

This will probably have a merge conflict with #21; I'll fix it when it happens. This required an edit to Reconciler to set the flag to false just before unmounting the component.

@coveralls
Copy link

coveralls commented Jan 29, 2018

Coverage Status

Coverage increased (+0.2%) to 85.928% when pulling 4683cb5 on AmaranthineCodices:setState-sanity-check into f0a9981 on Roblox:master.

expect(function()
Reconciler.reify(renderElement)
end).to.throw()
end)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can break this one into two tests, just like how willUnmount is a separate test

@@ -60,6 +60,9 @@ function Reconciler.teardown(instanceHandle)
Reconciler.teardown(instanceHandle._reified)
end
elseif Core.isStatefulElement(element) then
-- Stop the component from setting state in willUnmount or anywhere thereafter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, there are spaces for indentation here!

-- * During the component's render function
-- * After the component has been unmounted (or is in the process of unmounting, e.g. willUnmount)
if not self._canSetState then
error("State cannot be set at this point: are you setting state from an init, render, or willUnmount function?", 0)
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 a little clearer to say are you calling setState from instead of are you setting state from, since you can you set state from init as long as you don't call setState itself!

@LPGhatguy LPGhatguy merged commit 6c53c4f into Roblox:master Jan 29, 2018
@LPGhatguy
Copy link
Contributor

Thanks!

@LPGhatguy
Copy link
Contributor

Oops, I should've asked you to add an entry to the changelog!

I'll go ahead and do that for both of these PRs.

@AmaranthineCodices AmaranthineCodices deleted the setState-sanity-check branch January 29, 2018 18:51
@LPGhatguy
Copy link
Contributor

I know we've already merged this, but are there any other places where it might be inappropriate to call setState?

I reviewed some code today where setState was being called from willUpdate, which seems like a mistake (rendering shouldn't be re-entrant!)

@AmaranthineCodices
Copy link
Contributor Author

One place that immediately comes to mind is shouldUpdate. There is absolutely no reason to call setState from there.

wilUpdate and didUpdate are almost certainly bugs. I can think of some contrived situations where calling them wouldn't cause re-entrancy issues, but they're contrived and they're probably not worth allowing.

LPGhatguy pushed a commit that referenced this pull request Feb 17, 2018
This builds on the discussion from #23 after it was merged. It disallows calling setState in all lifecycle hooks, not just willUnmount.
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.

Give useful error messages when calling setState in the wrong places
3 participants