Assuming JS (this transfers to ReasonReact too): owner renders <Child onClick={this.onClick} />. Child renders <div onClick={this.onClick} />.
Upon div DOM click event:
- Child's onClick is called, sets its state for whatever purpose, then call this.props.onClick (from owner)
- Child re-renders following setState
- Owner's onClick sets owner's own state for whatever purpose
- Owner re-renders following setState
- Owner re-render causes Child to re-render again
The last step where Child is re-rendered again is the problem. This is deadly and is probably a big perf problems with react (relatively speaking only. React itself is very finely tuned).
Instead of Child calling setState, then call this.props.onClick(), swap the order (onClick, then setState). This way, reactjs batches the re-renders together, and owner re-renders first then Child re-renders, just once! This is easy to do in reactjs, but ReasonReact's setState is a bit implicit. You'd do it this way: onClick=reduce(_e => {onClick(); Click}). In the new self.send API, it'd be onClick=(_e => {onClick(); send(Click)}).
Q: is there any case where Child really needs to first setState, then call this.props.onClick? (Reminder: this causes the multiple re-renders problem above)
A: apparently not. So we should always just recommend the above solution.
Because the problem is exasperated by:
- ReasonReact recommending to put owner onClick inside reducer:
ReasonReact.UpdateWithSideEffects(newState, self => ownerOnClick()). Here, state update always happen first, then trigger side-effects. This always causes unbatching. This was a mistake. - subscriptions, whose update signals can be subscribed by different nodes in the tree, and thus way out of reactjs' ability to batch the updates into a single re-render. Imagine a whole tree paths of children subscribing to some external stores. In the worst case, the store fires update signals to all children, from leaf to root. The leaf would re-render once, then re-render again because its owner re-rendered, then again because its owner's owner re-rendered, etc. A path is O(log(N)) nodes, so a single leaf re-renders that many times, instead of once.
So, related tasks around the proposed solution:
- Avoid subscriptions as much as possible, pass down props
- Use the proposed solution above. Re-document this part of ReasonReact to encourage this practice.
- Maybe expose a better API to avoid this footgun altogether.
- Wait til Cristiano and Jordan come up with a less complex react update mechanism, with fewer (hopefully?) constraints than now (because it's kinda nice, allowing subscriptions, and not always worrying about this hardly well-encoded knowledge, and not always needing more APIs)
- rAF and debounce can solve it generally, but might not feel clean (?). Also, afaik inputs didn't work well under rAF batching or something. Or maybe that was just IE.
FAQ:
Q: In reactjs, does that mean we can should do this now? this.setState((freshState) => {this.props.onClick(); return newState})
A: no, you should not put side-effects inside a setState callback1 (setState can take a second callback. We'll designate the first one with callback1). And yes, this.props.onClick() is a side-effect, because (presumably) it makes an owner call its own setState, which puts that pending setState onto an internal update queue, which is a side-effect. What we recommended to do (to enable batching) isn't that, it's this: this.props.onClick(); this.setState((freshState) => newState). Which ReasonReact's API/docs don't nudge you to do well.
Q: what about setState(obj)? Anything we can explore there?
A: it's deprecated. Since a setState call is queued and not sync, this doesn't work:
setState({...this.state, foo: bar});
setState({...this.state, baz: qux}); // oops, you just spread the old foo: whateverOldValue here, instead of bar!Which is why is solves it:
setState((freshState) => {...freshState, foo: bar})
setState((freshState) => {...freshState, baz: qux})Q: regarding "expose a better API to avoid this footgun altogether", can't you just make UpdateWithSideEffects execute the side-effect callback (the 2nd arg) first, before using its first argument (the new state) & internally calling setState? (Or give a new API that works this way)
A: no. Your | MyAction => UpdateWithSideEffectOrNewAPI(newState, (selfWithFreshNewState) => someSideEffect()) happens the reducer (ofc). But when reducer is called, it's already too late. It goes like this: click event calls self.reduce's returned callback wrapper which wraps the your actual callback -> reduce calls the passed callback, getting the returned action -> ReasonReact calls reducer, passing the action, getting the new state config (along with side-effect description) -> internally, call setState(() => newState). See how there's no place to execute your side-effects first? Maybe this is more visual:
selfReduceMethod = (callback) => {
(event) => {
let action = callback(event); /* note: want the aforementioned batching? Gotta do it here, trigger owner setState, before our own right after */
thisJs##setState(freshState => {
let newStateAndSideEffectsConfig = component.reducer(action, freshState); /* <---- you need to pass freshState, can't lift this outside setState */
...
newState
})
}
}Q: ok, then why not just remove UpdateWithSideEffects, if the side-effect can be calling props (causing owner to setState)?
A: because it's actually good to have side-effects controlled and deferred with UpdateWithSideEffects. For example, under Fibers, the first arg (state) can be replayed several time, while the side-effect is executed once. It's still the case in our solution, where ownerOnClick() is executed once. But additionally, Fiber can ensure/ensures today that the side-effects are only performed when the tree is done re-rendering. (Edit: actually, according to Cristiano, we already do that today in RR). Anyways, it's good to encourage putting side-effects in UpdateWithSideEffects, but maybe being half-imperative and half-declarative is a bit awkward right now.
Q: since subscriptions have this problem, doesn't flux do?
A: yes, probably most of them. Thus the potential perf problems. The original flux avoided it by hooking into reactjs.
Q: is this un-batching solved in Fibers? Can we just put side-effects with owner state updates in UpdateWithSideEffect in Fibers?
A: Sema said this case might not be supported. Let's not worry about this in the medium-term.
Q: given that we use the (temporary) solution, any other pitfalls?
A: yeah... the problem is that when you do UpdateWithSideEffect(newState, self => propWhatever()), and if you're just expecting propWhatever to e.g. log stuff, it's fine for propWhatever to be in UpdateWithSideEffect. But, the owner might decide to finally do a setState call in the future! In which case you now have caused actual unbatching as per above. So maybe we should recommend the following simpler to remember guideline for now: pretend that any prop you call might do an owner setState (so put it in the correct place, aka before you set your state). Any other side-effect belongs in UpdateWithSideEffect. Maybe some type tricks can solve this too.