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

Consolidated React 16.3+ compatibility and roadmap discussion #950

Closed
markerikson opened this Issue May 13, 2018 · 22 comments

Comments

8 participants
@markerikson
Contributor

markerikson commented May 13, 2018

We've currently got several open PRs and discussions going on around how React-Redux is going to interact with React 16.3 and beyond:

I want to try to pull together some of the scattered discussion and thoughts and lay out some of the things we need to address, so that we can figure out the best path forward.

React 16.3+ compatibility concerns

  • 16.3 adds support for the new lifecycle methods, but doesn't actually start warning about them yet. It also adds <StrictMode> to check for async-unsafe usages in a subtree
  • 16.3 also adds the new context API. Old context will be removed in 17.0. Now that new context is available, we can use that instead, and that opens up a whole new set of possibilities for our internal implementation.
  • In order to be async-safe, we probably shouldn't have any fields on the component instance any more. There may also be issues with the way the memoized selector logic works right now, because it's running in componentWillReceiveProps, is stateful, and the calling logic relies on grabbing store.getState() every time.
  • connect() probably needs to use React.forwardRef() internally to allow easy access to instances of the wrapped component, and remove the need for getWrappedInstance()

Current PRs

We have three divergent 16.3-related PRs that rework connectAdvanced in separate ways:

  • #856:
    • drops the separate Subscription concept,
    • still uses old context
    • still subscribes to the store directly per component instance
    • removes makeSelectorStateful
    • adds a new shouldHandleSubscription value to let descendants know whether they need to subscribe themselves or not
    • uses functional setState to only return a state update if the selector indicates something changed
    • doesn't change the lifecycle methods
  • #919:
    • still has separate Subscription handling
    • still uses old context
    • still subscribes to the store directly per component instance
    • reworks makeSelectorStateful into makeUpdater
    • fixes the lifecycle method warnings by adding getDerivedStateFromProps, using react-lifecycles-compat to polyfill that, removing use of componentWillReceiveProps, and keeping the updater function in component state, as well as changing the HMR subscription updating to use componentDidUpdate
  • #898:
    • drops the separate Subscription concept, and also removes the special HMR update handling entirely as it's no longer needed
    • uses the new context API, creates a singleton Context.Provider/Consumer pair for internal use, and drops the old context API
    • changes from having every connected component instance subscribe to the store, to having only <Provider> subscribe, and put {storeState, dispatch} into the context
    • reworks makeSelectorStateful to accept the current store state as an argument instead of calling store.getState()
    • uses UNSAFE_componentWillReceiveProps and still runs the stateful selector there
    • still has the selector instance attached to the component instance
    • moves logic into a render prop callback for the Context.Consumer
    • currently loses the ability to pass the store directly as a prop to a connected component

Other Migration Concerns

Store Access via Context

There's a bunch of libs out there that access the store directly as this.context.store and do stuff with it. This is most common for libs that try to add namespacing behavior, where they intercept the original store in context and pass down their own wrapped-up version that only exposes a certain slice via getState(), and automatically adds namespacing to actions. I've also seen it used for libs that allow dynamically adding reducers/sagas from rendered components. (Examples: redux-dynostore-react-redux, redux-fractal, react-component-chunk, this gist from Sunil Pai, and #948 ).

Now, accessing the store in context is not part of our public API, and is not officially supported. Any of those uses will break as soon as we switch to using new context instead. But, given that we've got these sorts of use cases out there, I'd like to figure out if there's some way we can still make them possible, even if we don't officially support them.

Passing a Store as a Prop

In addition to the standard <Provider store={store} /> usage, connect has always supported <ConnectedComponent store={store} /> to pass a store to that specific component instance. That worked okay because each component instance subscribes to the store separately. The changes in #898 make that a lot harder to implement, because now <Provider> is the only subscriber, not the individual components.

The simplest resolution would be to just drop support for passing a store as a prop going forward, and tell people to wrap that one component in another <Provider>, like: <Provider store={secondStore}><ConnectedComponent /></Provider> Since a React Context.Consumer grabs its value from the nearest ancestor instance of the matching Context.Provider, that second Provider would be used instead of the one at the root of the app.

However, that would be a difference in behavior from passing the store as a prop, because store-as-prop only applies the new store to that one specific component instance, not any of its descendants.

The primary use case I've heard of for store-as-prop is to simplify testing of connected components, but I've also seen mentions of plugin-type behavior that relies on store-as-prop (per discussion in #942).

A couple possible workarounds or solutions here might be:

  • Have <Provider> accept a Context.Provider instance as a prop and use that if available, rather than the "default" singleton instance. connect() might also want to take a Context.Consumer instance as a prop.
  • Possibly in conjunction with that, in order to make store-as-prop work only for that one component, a connected component could generate its own unique context pair and actually render a <Provider> inside of itself, so that only its own consumer gets updates from that store. Seems silly and hacky, but it's the only immediate approach I can think of to keep the current semantics.

Actual React Suspense and Async Usage

Per discussion in #890 and #898, in the long term React-Redux is going to need to be rethought somehow to take full advantage of the async rendering capabilities React is adding. I wrote a summary of my understanding of the major issues, and Dan confirmed that was basically correct. Quoting:

To the best of my understanding, these are the problems that React-Redux faces when trying to work with async React:

  1. React's time-slicing means that it's possible for other browser events to occur in between slices of update work. React might have half of a component tree diff calculation done, pause to let the browser handle some events, and something else might happen during that time (like a Redux action being dispatched). That could cause different parts of the component tree to read different values from the store, which is known as "tearing", rather than all components rendering based on the same store contents in the same diff calculation.
  1. Because of time-slicing, React also has the ability to set aside partially-completed tree diffs if a higher priority update occurs in the middle. It would fully calculate and apply the changes from the higher-priority change (like a textbox keystroke), then go back to the partially-calculated low-pri diff, modify it based on the completed high-pri update, and finish calculating and applying the low-pri diff.
    In other words, React has the ability to re-order queued updates based on priority, but also will start calculating low-pri updates as soon as they're queued. That is not how Redux works out of the box. If I dispatch a "DATA_LOADED" action and then a "TEXT_UPDATED" action, the data load causes a new store state right away. The UI will eventually look "right", but the sequence of calculating and applying the updates was not optimal, because Redux pushed state updates into React in the order they came in.

The changes to use new context in #898 appear likely to resolve the "tearing" issues, but do nothing to deal with the update rebasing behavior in async React, or use with React Suspense.

Use of New Context and getDerivedStateFromProps

One of the downsides to the new context API is that access to the data in lifecycle methods requires creating an intermediate component to accept the data as props (per React docs: Context#Accessing Context in Lifecycle Methods, react#12397, and Answers to common questions about render props ).

Some of the discussion in #890 and #898 included notional rewrites of connect() to use an additional inner component class. I can see how that might help simplify some of the overall implementation, but it adds another layer to the component tree, and I'd really rather not do that (if for no other reason than it adds that many more levels of nesting to the React DevTools component tree inspector).

There's some open issues against the React DevTools proposing ways to hide component types (see React DevTools issues #503, #604, #864, #1001, and #997). If some improvements happened there, perhaps it might be more feasible to use this kind of approach.

Connection via Render Props

We've had numerous requests to add a render-props approach to connecting to the store, such as #799 and #920. There's also been a bunch of community-written implementations of this, such as redux-connector (which had some relevant discussion on HN including comments from me).

Clearly there's interest in this approach. I haven't actually written or used anything with render props yet myself, other than this first use of Context.Consumer, but I know that apparently it's possible to take a render-props implementation and wrap it in a HOC to make both methods a possibility.

Paths Forward

Tim ran a pair of Twitter polls asking if it was okay that React-Redux 6.0 only supported React 16.x, and if it went further and only supported 16.3+. In both cases, 90% of responses said "we're already on 16.x / 16.3, or can upgrade". Not scientific results, but a useful set of data points.

I'm going to propose this possible roadmap:

  • 5.1: Make additional improvements to the "remove async-unsafe lifecycle" changes in #919 so that there's no use of fields on the component instance, then merge that in. That would make 5.x at least not throw warnings as React 16.x progresses, and would likely be the last meaningful release in the 5.x line.
  • 6.0: flesh out my "use new context" PR in #898, including fixing any lifecycle issues, and ship that as 6.0. This would require React 16.3+ as a baseline. That gives us better compat for async behavior overall, and simplifies the polyfill/compat story. We'd also use forwardRef() here, drop getWrappedInstance() from the API, and probably drop store-as-prop as well.
  • 7.0: a full-on re-think of the React-Redux API, taking into consideration things like React Suspense and async rendering, render props, and so on. Not sure what the final result here would look like.

Based on that, our story is:

  • If you're on React 16.2 or below, stick with React-Redux 5.x. It still works fine, there's no reason for you to change, and we've at least made it stop warning if you do bump up to 16.3+.
  • If you're on 16.3+, go ahead and move up to React-Redux 6.x. Hopefully the switch from multiple subscriptions to a single subscription using new context will resolve the "tearing" concerns, and might even improve performance overall. This will require rewrites if you're accessing the store via context or using store-as-prop / getWrappedInstance, but otherwise keep your code the same.
  • If you're on 16.4+ and ready to move to the future, migrate to React-Redux 7.x once it's out. You'll probably have to rewrite your public usage of React-Redux, but once that's done you should be able to get all the benefits of async React.

Thoughts?

@markerikson

This comment has been minimized.

Contributor

markerikson commented May 14, 2018

I see a +1 from Dan in there, but I'll go ahead and tag @gaearon , @acdlite , and @bvaughn to get their thoughts from the React side of things, especially regarding the async-related aspects and stuff like the React DevTools.

@timdorr

This comment has been minimized.

Member

timdorr commented May 14, 2018

I've actually just started with a PoC for what 7.0 could look like. It's only about 30 lines of code, but leverages the new context API and uses a render prop (or cloneElement's a single child; neat!).

I still have to implement state selection and action creators, but it's pretty deceptively simple right now. While a bit more verbose on the user's end, there's far less hand-wavy magic going on and I think that tradeoff will be better for everyone.

Bonus points: it should be async compatible, though I need to stress test that later on.

@einarq

This comment has been minimized.

einarq commented May 15, 2018

Interesting read, thanks for sharing. Personally I feel the current version of react-redux is more or less "done". No need for any new features. So I hope you're able to create a new version that leverages the new capabilities of React without being hindered by trying to support all the community driven solutions out there. If people still need those they can stay on the version of react-redux they are already on (great if it doesn't throw warnings on 16.x tho of course). At the same time, the community is part of what makes redux great, so I see the conundrum. One great part of redux and react-redux is the simple API tho, so I guess keeping the API surface as small as possible going forward as well should be a high priority.
You're proposed plan looks really good to me.

Regarding connecting with the render-props pattern, I've found this simple trick to be useful in the cases where I've wanted that, so don't necessarily feel it is something react-redux itself needs to support.
https://reactrocket.com/post/turn-your-hocs-into-render-prop-components/

Thanks for all the awesome work on Redux! It's a great time to be a front-end developer :)

@mpeyper

This comment has been minimized.

mpeyper commented Jun 28, 2018

Hi, author of redux-subspace and redux-dynostore here.

@markerikson has repeatedly mentioned (read: blamed 😛) my libs as concerns for removing the store from the context entirely, so I wanted to jump into this conversation and help come to a conclusion on how we can maintain support, but still allow Redux to push forward and use all the fancy new feature React is offering us now (or soon).

I don't have much to offer, other than I am happy to answer any questions on our use case(s), why we did what we did, how a particular solution might work for us, or anything else you want to ask.

@markerikson

This comment has been minimized.

Contributor

markerikson commented Jun 28, 2018

FWIW, I know I've seen quite a few others that do similar things - yours just happen to be the ones that I remember the most and can point to :)

I'm starting to think that maybe we ought to still put the store into context, and if someone wants to intercept it and pass some kind of overridden version down, that's up to them. We'd still put the store state into context as well separately.

@mpeyper

This comment has been minimized.

mpeyper commented Jun 28, 2018

I'm starting to think that maybe we ought to still put the store into context

Selfishly, that is the easiest for us to upgrade to.

Would it make sense to have a seperate provider/consumer context pair for the store and have Provider set both up? Then connect (or whatever the future replacement may be) can use the regular one with just the state and dispatch and then more advanced use cases can use the "I hope you know what you're doing" one which provides the whole store. Just a thought to prevent people just using the store because it's there.

@xialvjun

This comment has been minimized.

xialvjun commented Jul 5, 2018

hope for api like this:

<Reudx.Provider store={store}>
  xxx
  <Redux.Consumer mapStateToProps={state => ({count: state.counter.count})} mapDispatchToProps={dispatch => ({ onInc: e => dispatch({ type: 'COUNT_INC' }) })}>
    {({ count, onInc }) => xxx}
  </Redux.Consumer>
  xxx
</Redux.Provider>

or even just:

<Reudx.Provider store={store}>
  xxx
  <Redux.Consumer selector={store => ({count: store.getState().counter.count, onInc: e => store.dispatch({ type: 'COUNT_INC' })})}>
    {({ count, onInc }, store) => xxx}
  </Redux.Consumer>
  xxx
</Redux.Provider>
@theJian

This comment has been minimized.

Contributor

theJian commented Jul 5, 2018

@xialvjun

{({ count, onInc }, store) => xxx}

I don't get the point for passing store as the second argument.

@xialvjun

This comment has been minimized.

xialvjun commented Jul 5, 2018

It's for people who is lazy to write selector.... @theJian

@xialvjun

This comment has been minimized.

xialvjun commented Jul 5, 2018

And in fact, the selector is not that useful if we write it like:

<Redux.Consumer>
  {store => {
    const count = store.getState().counter.count;
    const onInc = e => store.dispatch({ type: 'COUNT_INC' });
    return <div onClick={onInc}>{count}</div>
  }}
</Redux.Consumer>

I don't really know which is better. I just think we can use redux in the render-props way.

@xialvjun

This comment has been minimized.

xialvjun commented Jul 5, 2018

Maybe we can use PureComponent, then:

function render(store) {
  return xxx;
}
function component({ store }) {
  return render(store);
}

<Redux.Consumer selector={xxx} component={component} />
// or
<Redux.Consumer selector={xxx}>{render}</Redux.Comsumer>

fell free to ignore this.

@cellog

This comment has been minimized.

Contributor

cellog commented Jul 7, 2018

Regarding v7.0, based on previous open source experience, I highly recommend forking redux to redux2 or something, and provide a new unified way to access the store with redux or redux2 that third party libs can use to access it, and reach out to the most important ones with PRs. This way you can make a clean break to re-imagine how mapping state to the tree changes with React 16. I'll be watching closely, since my router ion-router is piggybacked on redux. Trying to maintain backwards compatibility directly doesn't really help anything in the long run, and in many cases in the short run. Better is to write the best thing and make migration easy (codemods anyone?)

In terms of responding to the new context model, what stops redux2 from putting the state in the root component (a context provider) using React state, and using context consumers to pass sub-trees of the state around? The redux consumers could accept "filters" to prune the tree to relevant portions needed for the component, and then re-rendering based on state change would be dead simple. Handling side effects and mutation in the new world will need some time to figure the new async world out, I think. But it would be a good opportunity to look at the situation afresh! Glad you're doing this Mark, thank you.

@nickmccurdy

This comment has been minimized.

Contributor

nickmccurdy commented Jul 7, 2018

I disagree with having a new package, I think that would prevent people from adopting it.

For example Koa originally started as plans for Express 5, but they decided to make a completely new package and project because of how much it affected back compatability. Unfortunately the popularity and community of Koa is still much lower than Express', despite Koa having a superior design and having the ability to do anything Express can in theory. Koa is significantly more difficult to adopt because the Express community didn't break back compatibility, which allows innovation while keeping related projects in sync over a single standardized API (even if the new API requires a lot of work to adopt, it's better for end users).

Let's keep the same package, even if it involves breaking back compatability for every single API. We could make the React specific version of Redux replace react-redux, or it can replace Redux itself and people can use a legacy version of Redux or a separate redux-legacy package if they aren't using React (Vue, backend only JS, non-web applications like Hyper, etc.).

@markerikson

This comment has been minimized.

Contributor

markerikson commented Jul 7, 2018

@cellog : a couple points of clarification.

First, the discussions here are primarily around React-Redux, not the Redux core. My ideal goal is that the Redux core won't need to change, and neither would the main React-Redux API (at least for a notional v6 that primarily is about compat with async React).

Second, maintaining backwards compatibility is very important for the immediate future. We've seen how things go when widely-used libraries or frameworks suddenly announce that everything is changing, and that's not something I want to do to Redux users. Again, I'd love it if a React-Redux user could just do yarn upgrade react@17 react-redux@6, and have their app just continue to work.

Third, your "context consumer and filters" sounds like... uh... connect and mapState :)

Looking at your specific lib, I see that you've got a store enhancer that adds extra fields to the store object itself, and uses context to access the Redux store directly. That's the kind of use case I'm concerned about, and want to find ways to support as we switch over to the new React context.

@markerikson

This comment has been minimized.

Contributor

markerikson commented Jul 7, 2018

My original train of thought for using new context would be that we'd only put the current store state and dispatch into context, to limit what people could do with the store. But, at this point I'm seriously considering just throwing the store itself into context along with its current state, and if someone wants to intercept and override it, that's up to them.

@cellog

This comment has been minimized.

Contributor

cellog commented Jul 7, 2018

clarifying myself: "context consumer" was referring to React Consumer returned from createContext() and filter was intended to be something similar to mapState but I was talking about putting the state into the component state of a Consumer. So this is not quite the same as creating the connect HOC. mapping that state to the dumb component underneath via selectors would be more like react-redux is now.

I was simply referring to the way redux interfaces with react in that it seems with react 16 and soon 17, it will be easier to maintain state without having to hoist it outside of react, and redux can use that strength.

It is good to maintain compatibility when it makes sense. Not disputing that. I'm simply questioning if the under-the-hood details really need that, or if providing a clear upgrade path for libraries that use under-the-hood stuff (like mine) is better. I'm also questioning if redux's real strength is its framework-agnostic nature. Do you have stats on how many people use it outside React?

As for my router, I will wait to see what you choose, then update based on that, like a good citizen :)

@markerikson

This comment has been minimized.

Contributor

markerikson commented Jul 7, 2018

I don't have specific stats, tbh, but I see no reason to suddenly make the core React-specific. Bindings layers for all the other major frameworks exist, and I don't want to make those impossible. Similarly, we've got a ton of example code that shows "hey, all you need to use Redux is one script tag and vanilla JS" - even if that's only a teaching use case, there's no reason to kill that off.

@cellog

This comment has been minimized.

Contributor

cellog commented Jul 10, 2018

FYI, I just converted ion-router to use the new context, and it gives me a bit more to work with. There is only one show stopper for me regarding the new context providers:

  1. enzyme doesn't yet support them, so you can't test it

So I can't release my work. I'm currently updating tests to match the new API anyways, so it's not huge, enzyme does seem to be within a few weeks of a release to support the new context, and fragments.

But the advantages are huge. I simply converted my top <Routes> into a context provider, which accepts the store as a prop, kind of like <Provider> does. If you decide to make Provider into a context provider, which will pass the store as a prop, I can convert <Routes> into a consumer and it will be even simpler. for the <Toggle> element, which is basically a connected component that switches display on/off based on redux state, turning that into a consumer would also be fantabulous. In the process of doing this, I killed off a lot of now-unnecessary code that was required to extend react-redux.

One of the big benefits I see from the new context design is that sub-apps with their own redux context can work without the need for a store key. The sub-app simply takes the provider that is closest in the higher part of the tree.

In any case, I am in favor of 7.x passing the whole store to the context consumers. This will allow store enhancers and middleware to work without modification, and vastly simplify extending react-redux. Obviously, I haven't looked at the async issues, and that is going to require a larger re-think.

@markerikson

This comment has been minimized.

Contributor

markerikson commented Jul 25, 2018

There's a couple React PRs that will likely be very relevant to v6.0:

The context-reading API is probably going to be necessary if we want to stick with a single wrapper component for connect instead of having to split the work into multiple components, since <Context.Consumer> uses render props. Meanwhile, I suspect the interaction tracking concept may be something we can use to tie together Redux store updates and React async rendering behavior.

@markerikson

This comment has been minimized.

Contributor

markerikson commented Aug 7, 2018

Pasting comment from #980 :

After discussion with both @cellog and @timdorr , we're leaning towards not putting out a 5.1 release. Trying to manage lifecycle behavior across multiple React versions is difficult (and apparently the react-lifecycles-compat polyfill uses the 16.3 behavior, not 16.4 , which doesn't help).

I'm currently trying to work on putting together a performance benchmark harness so that we can start comparing comparing behavior between different releases. From there, I plan to pick up work on #898 , or at least a variation of that approach, and start serious development on a 6.0 release that would probably be 16.5+ only.

@markerikson

This comment has been minimized.

Contributor

markerikson commented Sep 9, 2018

Tim put up a poll about a <Connect> / render-props-style API recently:

https://twitter.com/timdorr/status/1029451891082764290

Dead split 50/50 on whether people like it or not.

@timdorr

This comment has been minimized.

Member

timdorr commented Nov 6, 2018

Closing this out since we've got this set up on master. It'll be released soon-ish.

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