Skip to content
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

Bug report/feature request: Allow for optionally using React Context instead of Redux under the hood #1612

Closed
bergkvist opened this issue Jul 12, 2020 · 5 comments

Comments

@bergkvist
Copy link

Motivation/problem

When using react-redux 7, I experience a race condition when using deck.gl.

What most likely causes the issue is that react-redux doesn't prevent unrelated code from running in between a dispatch and a rerender. (in contrast to built in state management in React).

My gut tells me this might also be why react-redux feels "smoother" than React Context (which I suppose is what people complained about with react-redux v6).

To see the issue, click the world map and press the plus (+) key to start a zoom transition in the two examples below. In the react-redux example - the map will flicker as the zoom transition starts.

Hotswapping React Context and Redux

From what I understand, react-redux v6 used React Context under the hood - but react-redux v7 switched back to using Redux under the hood due to performance issues with React Context.

Since react-redux v6 does not provide useSelector and useDispatch hooks - I can't just switch back to the older version.

It would be nice if I could somehow hotswap the "under the hood" implementation of react-redux (between React Context and Redux).

Even more interesting would be to mix redux and context for different slices of state. (although I realize this is kind of crazy)

@markerikson
Copy link
Contributor

markerikson commented Jul 12, 2020

Sorry, not gonna happen any time soon. If React ever implements "context selectors", we'll investigate that and see if the performance issues have been resolved, but the implementations are far too different to try to maintain both of them (and especially when the context-based approach resulted in lousy performance).

See #1177 , as well as #1298 .

@bergkvist
Copy link
Author

bergkvist commented Jul 13, 2020

dai-shi seems to have been quite active in terms of working on userland implementations of context selectors for React. Would it be a bad idea to test out/rely on his use-context-selector implementation?

The benchmarks seem to indicate that performance is the same as react-redux v7.

In the examples below, I try to have a table component that is expensive to render depend on state dispatched from a different (draggable) component (panel-layout):

My hypothesis was that react-redux would handle this better because it doesn't force everything to rerender at the same time - meaning you might have a more smooth dragging experience.

If I'm going to be honest I don't really notice a significant difference in this example.

@markerikson
Copy link
Contributor

We're probably going to have to do a React-Redux v8 once useMutableSource is finalized. I don't know where the official context selectors implementation is on the React team's timeline, but it sounds like it'd be lucky to have that this year at best. Whenever that does come out, we'll certainly look at it.

I don't intend to do any other re-architecting beyond that.

I get that this is an issue for you, but you're basically asking us to gut the internals, again, for a very niche use case. If you or someone else would like to take the time and effort to put together a notional fork that actually implements the approach you're describing, I'd be willing to take a look at it. But, that's not something we have the time or interest in doing ourselves.

@dai-shi
Copy link
Contributor

dai-shi commented Jul 13, 2020

@bergkvist
While I don't read the original issue, here's my two cents.

use-context-selector v1 (as well as reactive-react-redux v4) uses undocumented changedBits=0 behavior. It would have been wonderful if we found this hack while we are still at react-redux v6.

However, react-redux v7 is already here which is totally valid in legacy mode. For concurrent mode, we are expecting useMutableSource to land in the near future. This is for external store like redux. It's likely that react-redux follows this pattern.


That said, I personally even think context is not a primary building block if we go with useMutableSource. My proposal in reactive-react-redux (not react-redux) is not to use context at all: dai-shi/reactive-react-redux#48

@bergkvist
Copy link
Author

bergkvist commented Feb 23, 2021

Turns out my original assumption here was wrong. The deck.gl-flickering issue was caused by a race condition that was fixed in react-redux v7.2.1. This also means I no longer have any desire/need for a React Context-based core in react-redux.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants