More setState checks #26
Conversation
lib/Component.spec.lua
Outdated
| expect(function() | ||
| local handle = Reconciler.reify(testElement) | ||
| -- Something of a hack, but... | ||
| handle._instance:shouldUpdate({}, {}) |
There was a problem hiding this comment.
The other thing you can do is capture the instances setState function from init, and then call that to trigger shouldUpdate/willUpdate/didUpdate.
It's quite a bit more verbose, but it avoids leaking internals into tests! I don't know if it's actually better; what do you think?
There was a problem hiding this comment.
It's a lot better, because then I'm not depending on the format of the instance handles (oh no, we changed the reified handle format, our tests for setState broke!). I'll make the change.
lib/Component.lua
Outdated
| self:didMount() | ||
| self._canSetState = true |
There was a problem hiding this comment.
Should we disallow setState in didMount and didUpdate? These might be less clear-cut.
I've definitely written code that triggers setState from inside those two lifecycle events, especially for code with network resources, where the flow is:
renderonce without the data (returning nil)didMounttriggers a network request on another coroutine callssetStateto mark the component as loadingrenderagain, this time showing a spinnerdidMountdoes nothing
There was a problem hiding this comment.
I can see use-cases for didMount and didUpdate. I can also see arguments for disallowing them, most of them centered around "avoiding footguns". I don't know that that's a very good reason to stop the sort of paradigm that you demonstrate, to be honest. Limiting the power of Roact just to avoid an error case that will still throw an error (stack overflow, I believe) seems like a poor decision on my part - my bad.
We should address in the documentation that you have to be careful about calling setState within didMount/didUpdate to avoid infinite loops - setting state in these hooks should be the exception, not the norm for the component's behavior.
There was a problem hiding this comment.
I agree. Asynchronous rendering with an update queue would solve any of the stack overflow problems, but does change behavior with regards to re-rendering before your parent component finishes mounting.
lib/Component.spec.lua
Outdated
| expect(function() | ||
| Reconciler.reify(testElement) | ||
| shouldUpdate() | ||
| end).to.throw() |
There was a problem hiding this comment.
Since shouldUpdate is implemented entirely by the component, _canSetState won't be checked if we call shouldUpdate directly, right?
I wonder if we should call the component's setState instead of force updating, to hit paths like this one?
There was a problem hiding this comment.
This is true! It looks like this test case only passed to begin with when it re-entered shouldUpdate. I've fixed that now.
lib/Component.lua
Outdated
| if not self._canSetState then | ||
| error("setState cannot be used currently: are you calling setState from an init, render, or willUnmount function?", 0) | ||
| error("setState cannot be used currently: are you calling setState within a lifecycle hook?", 0) |
There was a problem hiding this comment.
I like mentioning render and init explicitly here since they aren't lifecycle hooks.
There was a problem hiding this comment.
I've changed the error message to be more descriptive:
setState cannot be used currently, are you calling setState from any of:
* the willUpdate or willUnmount lifecycle hooks
* the init function
* the render function
* the shouldUpdate function
|
I've been on vacation for the past two weeks -- I'll get back into helping with this PR! |
lib/Component.lua
Outdated
| * the willUpdate or willUnmount lifecycle hooks | ||
| * the init function | ||
| * the render function | ||
| * the shouldUpdate function]] |
There was a problem hiding this comment.
Lua ignores the first newline after [[ I'm fairly sure -- we should put one there to make the message more coherent!
There was a problem hiding this comment.
Oops, this should be fixed now.
| @@ -182,6 +188,8 @@ function Component:_forceUpdate(newProps, newState) | |||
| ) | |||
| end | |||
|
|
|||
| self._canSetState = true | |||
There was a problem hiding this comment.
An interesting implication here: the state of your parent can't have its state updated while you're in the middle of updating. Calling a function in your didUpdate callback that indirectly invokes setState on a parent component would thus fail.
I wonder if that's a case that we hit in real code, and if that's a bug!
There was a problem hiding this comment.
This will only throw an error if this setState indirection happens synchronously. If you spawn another thread and trigger the parent setState call there, or do it from an event connection, there won't be an error.
Since most use-cases I can see for calling setState in didUpdate involve some sort of blocking operation (that stalls out the entire render until it's complete if it's done synchronously anyway), I don't think this is a major issue.
Hypothetically this could be worked around by deferring didUpdate invocations until after the entire tree has re-rendered, parents and all, but I don't think this is worth the effort.
| @@ -184,6 +184,10 @@ return function() | |||
| }) | |||
| end | |||
|
|
|||
| function InitComponent:render() | |||
| return nil | |||
| end | |||
There was a problem hiding this comment.
This is an interesting artifact of a test that's only as coarse as "this high level call should throw". I wonder if there's a way to make these kinds of tests more robust in the future.
This builds on the discussion from #23 after it was merged. It disallows calling
setStatein all lifecycle hooks, not justwillUnmount. The error message has been changed to reflect this: