roact icon indicating copy to clipboard operation
roact copied to clipboard

Setting self.state to something other than a table only fails when calling setState

Open MagiMaster opened this issue 5 years ago • 4 comments

For example, if you set self.state to a string, nothing fails immediately. When you later call self:setState, you get a "table expected, got string" error, but attached to the setState line, which makes it hard to trace what went wrong. (Roact should probably raise an error if self.state is not a table after init().)

MagiMaster avatar Apr 04 '20 01:04 MagiMaster

Digging around a bit, it looks like this is partially related to the logic in assign: https://github.com/Roblox/roact/blob/f55538b529f59683eec1ee75bd8735bb6fdefc98/src/assign.lua#L13-L21

If all of the inputs that we're copying from are empty or nil, then we never even attempt to copy anything to the target and we don't get any errors.

After calling init, we assign the result of getDerivedStateFromProps back onto it: https://github.com/Roblox/roact/blob/f55538b529f59683eec1ee75bd8735bb6fdefc98/src/Component.lua#L319-L324

If getDerivedStateFromProps returns something, then we'll encounter this error sooner. Broadly speaking, users can avoid issues like this by not assigning directly to self.state. But since we need that as a legacy behavior, we should also do a check after we run init that the user left the state as a table or nil.

ZoteTheMighty avatar Apr 06 '20 16:04 ZoteTheMighty

I thought self.state = {} was still the recommended way to set the initial state in init()?

MagiMaster avatar Apr 06 '20 18:04 MagiMaster

The best practice as of Roact 1.x is to call setState instead of assigning directly. https://roblox.github.io/roact/api-reference/#init (as well as the examples in the "Guide" section of the docs) should be up-to-date with this.

I suspect this change hasn't been communicated particularly well, though, so we may want to find more ways to promote the new best practice.

All of that said, we should still fix this issue by adding a check in the appropriate place after calling init.

ZoteTheMighty avatar Apr 06 '20 18:04 ZoteTheMighty

Is adding a check just after init a sound enough solution?

This condition could happen any time a user reassigns state. We did have the convention of reassigning it in init for a long time, but there's nothing stopping a user from reassigning state (or any other member) to a different value that will later cause issues.

I believe this problem will largely be solved by Luau types once we get them, which will solve this issue in all cases.

LPGhatguy avatar Apr 08 '20 20:04 LPGhatguy