New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can we avoid inconsistencies on non-batched dispatches? #292

Closed
gaearon opened this Issue Feb 17, 2016 · 28 comments

Comments

@gaearon
Copy link
Contributor

gaearon commented Feb 17, 2016

Prompted by reduxjs/redux#1415, please read the discussion there. We currently subscribe in componentDidMount but it runs from children first. This has a potential of introducing inconsistencies when a child receives some update state earlier than its parent that passes a state-dependent prop to it.

Would subscribing the parents first fix the inconsistencies? Can we do that somehow (e.g. by passing subscribers up via context)?

Alternatively, can/should we wrap dispatch into ReactDOM.unstable_batchedUpdates() by default like Relay does?

cc @epeli @chandlerprall @tappleby @acdlite

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Feb 17, 2016

Alternatively, can/should we wrap dispatch into ReactDOM.unstable_batchedUpdates() by default like Relay does?

Obvs this wouldn’t work for React Native. 😁

@epeli

This comment has been minimized.

Copy link
Contributor

epeli commented Feb 17, 2016

If I understand correctly reduxjs/redux#1415 is basically dup of #99?

Alternatively, can/should we wrap dispatch into ReactDOM.unstable_batchedUpdates() by default like Relay does?

I think so. I've been using redux-batched-updates in all of my projects and there have been zero issues. More often my apps tend to break if I'm not using it... It will also boost performance in some cases.

But the biggest argument for enabling it by default in my opinion is that it makes dispatch() consistent. Currently by default dispatch() can surprise you because it works differently depending on when you happen to call it. Ex. onClick vs. setTimeout.

Is there actually any downsides if it's made default?

Obvs this wouldn’t work for React Native.

Any ideas why it is in ReactDOM and not in React itself?

I haven't confirmed that the issue is actually present in React Native but I'm now working on a React Native app so I can easily test if it's not confirmed yet?

@epeli

This comment has been minimized.

Copy link
Contributor

epeli commented Feb 17, 2016

Is there actually any downsides if it's made default?

Hah. I just realized I might have answered it myself:

Any ideas why it is in ReactDOM and not in React itself?

Redux nor react-redux does not depend on react-dom and for a good reason because it's not wanted in React Native or any other non-dom React projects...

Any other reasons?

@epeli

This comment has been minimized.

Copy link
Contributor

epeli commented Feb 17, 2016

Would subscribing the parents first fix the inconsistencies? Can we do that somehow (e.g. by passing subscribers up via context)?

This is an interesting idea!

I think I actually proposed at some point that the render should be triggered only from the most top-level connect()ed component but I quickly realized that it would not work because some of the nested connect()ed components would not update in that case. But if we can intercept all the updates during a tick and manually run those updates from the top to the leaves at the end of that tick using some context trick it might just work!

But on the other hand it would be a huge hack and there has been some talking about making the update batching default on all cases so I don't think it's worth it.

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Feb 17, 2016

I’m just thinking about something like

getChildContext() {
  return {
    subscribeParentConnectedComponents: this.trySubscribe
  }
}

componentDidMount() {
  this.trySubscribe()
}

trySubscribe() {
  if (!this.unsubscribe) {
    if (this.context.subscribeParentConnectedComponents) {
      this.context.subscribeParentConnectedComponents()
    }
    this.unsubscribe = this.context.store.subscribe(...)
  }
}
@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Feb 18, 2016

OK, so I tried reversing the order. It doesn’t really help.

If the parent calls setState() first, then the child will be re-rendered synchronously, and the child will have stale data as store props. If the child calls setState() first, then we have the same problem, but in reverse way (child store props are up to date but props received from parent are not).

So it’s not really about the order. No matter what order subscribers are called, one of them is going to come after another, and it will cause staleness unless you explicitly tell React to batch updates.

It seems like we can’t really do anything here. At least not by ensuring the subscription order.

I’ll ask about unstable_batchedUpdates() and what’s the RN situation there, but for now I’m closing because most likely this isn’t actionable, and the workaround with calling unstable_batchedUpdates() yourself works well enough when you really need it.

@gaearon gaearon closed this Feb 18, 2016

@josephsavona

This comment has been minimized.

Copy link

josephsavona commented Feb 18, 2016

@gaearon We encountered this same issue early on in development of Relay and are using unstable_batchedUpdates to prevent such discrepancies at the framework level. We have a relayUnstableBatchedUpdates module that is forked to use the appropriate version depending on web/native - see the two versions at https://github.com/facebook/relay/tree/master/src/tools.

@chandlerprall

This comment has been minimized.

Copy link

chandlerprall commented Feb 18, 2016

Thanks everyone for taking a look and weighing in! I'm very interested if you find out more details about incorporating unstable_batchedUdpates @gaearon

@happypoulp

This comment has been minimized.

Copy link

happypoulp commented Feb 18, 2016

May the present issue be related to that react issue: facebook/react#2410 ?

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Feb 18, 2016

Is adding .native.js to the module name enough to make React Native prefer it?

@gaearon gaearon reopened this Feb 18, 2016

@josephsavona

This comment has been minimized.

Copy link

josephsavona commented Feb 18, 2016

@gaearon yup!

@josephsavona

This comment has been minimized.

Copy link

josephsavona commented Feb 18, 2016

Cc @skevy who worked to get the web/native compatibility implemented in Relay.

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Feb 18, 2016

This would force us to assume ReactDOM outside of RN though. Which might be fine because I don't think people use ReactRedux with any other renderers.

@tappleby

This comment has been minimized.

Copy link

tappleby commented Feb 18, 2016

I ran into issues with wrapping dispatch w/ unstable_batchedUdpates. The return values from dispatch is inconsistent: acdlite/redux-batched-updates#1, this was the main reason I ended up writing redux-batched-subscribe.

My only idea is having each connected component being responsible for notifying its own children instead of subscribing directly to the store, this would end up with only the root connected component being subscribed to redux.

@epeli

This comment has been minimized.

Copy link
Contributor

epeli commented Feb 18, 2016

This would force us to assume ReactDOM outside of RN though. Which might be fine because I don't think people use ReactRedux with any other renderers.

I've actually considered using react-blessed. Could we wrap the require in try-catch?

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Feb 18, 2016

See #293.

@epeli

This comment has been minimized.

Copy link
Contributor

epeli commented Feb 18, 2016

Oh, it's now using ES2015 imports. Try-catch will not work with it. Would it be feasible to opt-in to the old school require and try-catch for that case only?

@skevy

This comment has been minimized.

Copy link

skevy commented Feb 18, 2016

Using try/catch would break the RN packager, as well as any other packager that tries to statically analyze requires.

try/catch around requires should really be avoided unless you're only targeting a Node environment.

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Feb 18, 2016

Yeah. For example Webpack allows it but will print annoying warnings.

@matianfu

This comment has been minimized.

Copy link

matianfu commented Feb 26, 2016

@gaearon

If

  1. only root component subscribe to redux store.
  2. all component passing store to all children in JSX, something like:
<MyComponent store={this.state.store} ... />
  1. all container components do this.setState({ store: nextprops.store, ... }) in componentWillReceiveProps(nextprops)

Do you think the problem will be solved?

This is basically the React way. I think react-redux should NOT setState() outside the React work flow (reconciliation). Store state should only be passed by props in top-down manner, and setState() should only be called in componentWillReceiveProps(). The root cause of the problem could be considered that redux uses a single JavaScript object, and redux emit change events too early.

Besides, the reference equality check, the gem of redux, could be done in shouldComponentUpdate(). Here, both store state and props is synchronized, and unnecessary render() can be prevented.

If this works, it looks much cleaner than current implementation in react-redux.

@matianfu

This comment has been minimized.

Copy link

matianfu commented Feb 26, 2016

Please neglect the post above. It's not correct. Passing store explicitly via props from root to leave essentially trigger a full re-render and nothing can be done in shouldComponentUpdate(), otherwise the store state won't be populated to all nodes.

@matianfu

This comment has been minimized.

Copy link

matianfu commented Feb 27, 2016

Another Day, Another Proposal, :LOL:

If the parent calls setState() first, then the child will be re-rendered synchronously, and the child will have stale data as store props. If the child calls setState() first, then we have the same problem, but in reverse way (child store props are up to date but props received from parent are not).

So it’s not really about the order. No matter what order subscribers are called, one of them is going to come after another, and it will cause staleness unless you explicitly tell React to batch updates.

It seems like we can’t really do anything here. At least not by ensuring the subscription order.

This time. I propose the redux calls all component's handleChange() twice, and the Component knows if it's called in the first round, or the second one.

In the first round, redux just tells all Components that a state change occurred, but no Component should call setState() method. This operation is postponed to the second round, and in second round, not all Component's handleChange() are called, depending on what value they return in first round.

In the first round, each Component do a reference equality check here. If it is confident that the store state change is totally irrelevant to itself, it can set this.state to latest store state directly and tells redux that don't call me in second round. This violates React's convention, but if we do not have only one object as the whole app's state, this is reasonable.

If it is not confident about that, it saves the new store state to a local variable. This indicates that the Component need to be reconciled but the action is postponed to the second round. It tells this to redux by return value, for example, return true.

There may be a possible pitfall here, the assumption should be: even if later, in the second round, the ancestor or parent's setState() fires, a new props is passed down, it is irrelevant to the states anymore. If not sure, choose the second choice.

In the second round, redux only calls handleChange() to Components that returns true in the first round. redux should make sure parent is always called before children.

In this way, the stale props problem won't happen. And if the child has a stale store state, now it already has a copy of new store state.

For the parent-coupled children, there are two possibilities to update its own state. One is when parent setState() fires, and a props is passed down. The children do setState() in its own componentWillReceiveProps() method. The other, the children do this in second round handleChange().

In either case, props is up-to-date and newest state is available. After the processing, the local variable storing newest state should be cleaned.

In batched setState(), it is possible setState() is called twice. But in the second time, the Component knows that it already has the newest state and the reconciliation has been done (new store equals this.state.store and there is no local copy of newest store). It can simple neglect it.

If this works, most node can use shouldComponentUpdate() to prevent unnecessary re-rendering, it's better than memoization in most case.

Hope to hear your comments, if it will work, this time.

matianfu

@slorber

This comment has been minimized.

Copy link

slorber commented Apr 30, 2016

@tappleby

My only idea is having each connected component being responsible for notifying its own children instead of subscribing directly to the store, this would end up with only the root connected component being subscribed to redux.

I agree with that.

It looks to me an antipattern to have Parent > Child, both using connect, and parent passing down props to child. If Child has to receive props from Parent, the parent could pass as props everything the child needs. I don't really see any advantage of the child receiving state from both props and connect at the same time and it's probably better to choose one or the other but not both at the same time.

@epeli

But the biggest argument for enabling it by default in my opinion is that it makes dispatch() consistent. Currently by default dispatch() can surprise you because it works differently depending on when you happen to call it. Ex. onClick vs. setTimeout.

Do you mean that dispatches from event handlers are batched automatically? If that's the case yes it's probably worth batching automatically on dispatches to have a consistent behavior, even if I still think the issue described here can be avoided in the first place.

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Apr 30, 2016

AFAIK the plan in React is to always batch eventually so this might become a non-issue later.

@jimbolla

This comment has been minimized.

Copy link
Contributor

jimbolla commented Jun 17, 2016

I have an idea that might solve this. It would require tweaking the way subscribe() works. subscribe(listener) would become subscribe(listener, [priority]). Then when the store state changes, listeners would fire by priority in ascending order. (subscribe should probably store them in that order).

Now, once subscribe() has that, connect can make use of another context field...let's call it connectedComponentDepth. It would use that depth as its priority when subscribing, and also redefine it as a childContext with value +1. Given this, parent components would always have a better priority than their children and so would have its listener invoked first.

@epeli

This comment has been minimized.

Copy link
Contributor

epeli commented Jun 17, 2016

@slorber

Do you mean that dispatches from event handlers are batched automatically?

Yes.

@timdorr timdorr closed this Aug 14, 2016

dk00 added a commit to dk00/linking that referenced this issue Dec 9, 2016

Notify components in hierarchical order
  Components used to subscribe to store updates in componentDidMount, and it
runs from lowest hierarchy. A child could subscribe earlier and receive
update earlier than its parent, but it might recieve new props, or be
removed after update of its parent.

  Changes are made to fix inconsistencies, by notifying components in
hierarchical order. All components that subscribe to store updates now
create a new listener collection for its children, and notify after update.
This fixes reduxjs/react-redux#292 .

  Updates to components are initiated by calling top level listeners, and
these components notify their children after handling changes.
reduxjs/react-redux#398 is also fixed by send only necessary notifications:
When a component don't have to update in response to state changes, its
children is notified by calling the listeners. When a component need to
update, its listeners are not called, use simply setState(empty), and
let React handle the rest.

@jimbolla jimbolla referenced this issue Dec 11, 2016

Closed

v5 release chores #473

7 of 7 tasks complete
@qbolec

This comment has been minimized.

Copy link

qbolec commented Sep 5, 2017

Is it fixed in any of 4.x.y versions?
Is it fixed in any of 5.x.y versions?
(I find it difficult to tell what is the semantic of "Closed")

@markerikson

This comment has been minimized.

Copy link
Contributor

markerikson commented Sep 5, 2017

@qbolec : Yes, React-Redux v5 introduces top-down subscriptions, which resolves the issues described here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment