Not a member of Pastebin yet?
Sign Up,
it unlocks many cool features!
- ## Problem Setup
- 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).
- ## Solution
- 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.
- ## Why talking about this over-rerender problem now?
- 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:
- ```js
- 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:
- ```js
- 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:
- ```js
- 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.
Add Comment
Please, Sign In to add comment