Allow setState inside willUpdate and init #139
Conversation
|
This probably isn't quite ready (tests, docs, and changelog entry needed still), but probably ready for someone to see if this is sound. |
|
This is interesting. Is this a departure from the direction React is going in with respect to their newer versions of lifecycle methods like Is the idea that it's better to adjust the re-rendering rules to account for situations where this is useful, rather than trying to build ways around the way they already are? Does it jive with your plans for the new reconciler in some way? |
|
The original intent with locking down React handles |
|
Okay, I think I see where you're coming from. It definitely would be good to have fewer situations where users have to worry about creating infinite re-rendering scenarios, and I suspect This looks fine so far, I'll look again and review when you tick some boxes. |
LPGhatguy
changed the title
docs/api-reference.md
Outdated
| end | ||
| ``` | ||
|
|
||
| In older versions of Roact, `setState` was disallowed in `init`, and you would instead assign to `state` directly. This is still acceptable, but it's simpler to use `setState`: |
There was a problem hiding this comment.
Wording right before the colon here is a bit confusing as it sounds like you're about to demonstrate use of setState. Could you construct the sentence differently? Maybe "It's simpler to use setState, but it's still acceptable to assign it directly:"
docs/api-reference.md
Outdated
| * Lifecycle hooks: `willUnmount` | ||
| * Pure functions: `render`, `shouldUpdate` | ||
|
|
||
| Calling `setState` inside of `init` or `willUpdate` has different behavior in other places. Because Roact is already going to render or update a component in these cases, the update will be replaced instead of another update happening. |
There was a problem hiding this comment.
I don't like the "in other places" part (also, you're missing a "than"). It might be fine to just say "...has special behavior." What do you think?
docs/guide/state-and-lifecycle.md
Outdated
|
|
||
| In this case, we're passing a _function_ to `setState`. This function is called and passed the current state, and returns a new state. It can also return `nil` to abort the state update, which lets Roact make some handy optimizations. | ||
|
|
||
| Right now, this version of `setState` is exactly the same as the form where we pass an object. In the future, Roact will support a number of features, like asynchronous rendering, that make the distinction more important. |
There was a problem hiding this comment.
I'd maybe say "works exactly the same"
lib/Component.spec.lua
Outdated
| end).to.throw() | ||
| local handle = Reconciler.mount(testElement) | ||
|
|
||
| expect(forceUpdate).to.be.a("function") |
There was a problem hiding this comment.
Not a problem, but I'm curious why you added this.
There was a problem hiding this comment.
My thought process was that this expectation is sort of implicit, and if it ever failed, the error message would be not-so-great. Thinking about it now, I think I'll remove it.
This change lays a little bit of groundwork for allowing
setStatein more places.Right now,
setStateis disallowed in all lifecycle hooks andrenderwith the exception ofdidMount. I think it's safe to expand this to allowwillUpdateas well, predicated on the behavior ofsetStatenot causing a re-render at that point, but instead updating the existing state.This PR adds
_setStateWithoutUpdatenext to_setStateBlockedReason, which changes the behavior of a state update.Checklist before submitting:
CHANGELOG.md