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

React-Redux Roadmap: v6, Context, Subscriptions, and Hooks #1177

Closed
markerikson opened this issue Feb 3, 2019 · 216 comments

Comments

@markerikson
Copy link
Contributor

commented Feb 3, 2019

Update: React-Redux v7 final is now available!

We've just published v7 final, with the changes covered in this thread. Upgrade your apps today!

Back in December, we released React-Redux version 6.0. This release featured a major rearchitecture of the implementations for <Provider> and connect, centered around our use of React's context API.

We put the pre-release builds through extensive tests, including our existing unit test suite and multiple performance benchmark scenarios. We also requested early adopter feedback on beta versions. Based on all that information, we determined that v6 was ready for public release, and published it.

I think most library maintainers would agree that the best way to guarantee getting bug reports is to publish a new major version, because A) that means the entire community is about to start using the new code, and B) it's impossible for a maintainer to anticipate every way that the library is being used. That's definitely been true with React-Redux version 6.

Version 6 does work as intended, and many of our users have successfully upgraded from v5 to v6, or started new projects with v6. However, we've seen a number of concerns and challenges that have come up since v6 was released. These relate to both the current behavior of v6 and its API, as well as future potential changes such as the ability to ship official React hooks that use Redux.

In this issue, I want to lay out a description of the changes we made in v6 and why we made them, the challenges we are currently facing, the constraints that potential solutions need to conform to, and some possible courses of action for resolving those challenges.

TL;DR:

  • v6 switched from direct store subscriptions in components, to propagating store state updates via createContext
  • It works, but not as well as we'd hoped
  • React doesn't currently offer the primitives we need to ship useRedux() hooks that rely on context
  • Based on guidance from the React team, we're going to switch back to using direct subscriptions instead, but need to investigate an updated way to re-implement that behavior
  • As part of that, we need to improve our tests and benchmarks to ensure we're covering more use cases from throughout the community
  • Our ability to ship public hooks-based APIs depends on switching back to subscriptions, and how we update connect may require a major version bump.
  • We need volunteers and contributions from the React-Redux community to make all this happen!

Implementation Changes in Version 6

In my blog post The History and Implementation of React-Redux, I described the technical changes we made from v5 to v6, and why we made them. Summarizing those changes:

  • v5 and earlier:
    • The store instance was put into legacy context
    • Each connected component instance was a separate subscriber to the Redux store
    • Because components subscribed directly, they could also accept a store instance as a prop named store, and use that instead of the instance from context
  • In v6:
    • The current Redux store state is propagated via React's new createContext API
    • Only <Provider> subscribes to the store - the components just read the store state from context
    • Since components don't subscribe directly, passing store as a prop is meaningless and was removed

We made these changes for several reasons:

  • Legacy context will eventually be removed, and can cause problems when used side-by-side with the new context API
  • The React team has warned that React's upcoming "Concurrent Mode" and "Suspense" capabilities will cause many existing state patterns to break or behave unpredictably. By putting the store state itself into context, React ensures the entire component tree sees the same state value consistently, and therefore React-Redux would hopefully would be more compatible with Concurrent Mode when it comes out.
  • v5 had to specifically implement custom tiered subscription logic to enforce top-down updates in order to fix "zombie child" bugs (where a child component subscribes before its parent, has its data deleted from the store, and then tries to read that data before the parent stops rendering it). Context updates top-down by default as React renders, allowing us to remove our own custom code for this issue.

Challenges with v6

Performance

During the development of v6, we put together a performance benchmarks suite that we could use to compare the behavior of different builds of React-Redux. These benchmarks are artificial stress tests that don't necessarily match real-world app setups, but they at least provide some consistent objective numbers that can be used to compare the overall behavior of builds to keep us from accidentally shipping a major performance regression.

Our comparisons showed that v6 was generally slower than v5 by different amounts in different scenarios, but we concluded that real-world apps would probably not experience any meaningful differences. That seems to have been true for most users, but we have had several reports of performance decreases in some apps.

Fundamentally, this is due to how v6 relies on context for propagating state updates. In v5, each component could run its own mapState function, check the results, and only call this.setState() if it knew it needed to re-render. That meant React only got involved if a re-render was truly necessary. In v6, every Redux state update immediately causes a setState() in <Provider> at the root of the tree, and React always has to walk through the component tree to find any connected components that may be interested in the new state. This means that v6 ultimately results in more work being done for each Redux store update. For usage scenarios with frequent Redux store updates, this could result in potential slowdowns.

store as Prop

We removed the ability to pass a prop named store to connected components specifically because they no longer subscribe to stores directly. This feature had two primary use cases:

  • Passing store instances to connected components in tests without needing to wrap them in a <Provider>
  • Rare situations where a specific component needed to subscribe to a different store than the rest of the application component tree around it

Our general guidance for the first use case was to always wrap components in <Provider> in tests. We tried to provide a solution for the second use case by allowing users to pass custom context instances to both <Provider> and connected components.

However, since the release of v6, we've had several users express concerns that the removal of store as a prop breaks their tests, and that there are specific problems with trying to use the combination of Enzyme's shallow() function with a <Provider> (and React's new context API in general).

Context API Limitations

At first glance, React's new createContext API appears to be perfectly suited for the needs of a library like React-Redux. It's built into React, it was described as "production-ready" when it was released, it's designed for making values available to deeply-nested components in the tree, and React handles ordering the updates in a top-down sequence. The <Context.Provider> usage even looks very similar to React-Redux's <Provider>.

Unfortunately, further usage has shown that context is not as well suited for our use case as we first thought. To specifically quote Sebastian Markbage:

My personal summary is that new context is ready to be used for low frequency unlikely updates (like locale/theme). It's also good to use it in the same way as old context was used. I.e. for static values and then propagate updates through subscriptions. It's not ready to be used as a replacement for all Flux-like state propagation.

React-Redux and Hooks

In addition to concerns about performance and state update behaviors, the initial release of the React Hooks API will not include a way to bail out of updates triggered by a context value update. This effectively blocks React-Redux from being able to ship some form of a useRedux() hook based on our current v6 implementation.

To again quote Sebastian:

I realize that you've been put in a particularly bad spot because of discussions with our team about concurrent mode. I apologize for that. We've leaned on the use of context for propagation which lead you down the route of react-redux v6. I think it's generally the right direction but it might be a bit premature. Most existing solutions won't have trouble migrating to hooks since they'll just keep relying on subscriptions and won't be concurrent mode compatible anyway. Since you're on the bleeding edge, you're stuck in a bad combination of constraints for this first release. Hooks might be missing the bailout of context that you had in classes and you've already switched off subscriptions. :(

I'll pull out one specific sentence there for emphasis:

I think [v6's use of context is] generally the right direction but it might be a bit premature.

Constraints

Whatever solutions we come up with for these challenges need to fit within a variety of overlapping constraints.

Performance Should Match or Improve vs v5

Ideally, v6 should be at least as fast as v5, if not faster. "Faster", of course, is entirely dependent on what metrics we're measuring, and how we're measuring them.

Handle Use Cases for store as Prop

We need to support the use cases that were previously handled by passing a store directly as a prop to connected components. As part of that, we should ensure that we have tests that cover these usages.

Future React Compatibility

We have some idea what the potential concerns are around React's future Concurrent Mode and Suspense capabilities, but it would help to have some concrete examples that we can use to ensure we're either not breaking application behavior, or at least can help us quantify what the potential breakages are.

Quoting Dan Abramov:

We need to be clear that there are several “levels” of compatibility with new features of React. They’re not formalized anywhere yet but a rough sketch for a library or technique X could be:

  1. X breaks in sync mode
  2. X works in sync mode but breaks in concurrent mode
  3. X works in concurrent mode but limits its DX and UX benefits for the whole app
  4. X works in concurrent mode but limits its UX benefits for the whole app
  5. X works in concurrent mode but limits its DX and UX benefits for features written in X
  6. X works in concurrent mode but limits its UX benefits for features written in X
  7. X works in concurrent mode and lets its users take full advantage of its benefits

This is not a strict progression and there’s a spectrum of tradeoffs. (For example, maybe there is some temporary visual inconsistencies such as different like counts, but no crashes are guaranteed.)

But we need to be more precise about where React Redux is, and where it aims to be.

At a minimum, we should ensure that React-Redux does not cause any warnings when used inside a <StrictMode> component. That includes use of semi-deprecated lifecycle methods like componentWillReceiveProps (which was used in v5).

Don't Re-Introduce "Zombie Child" Problems

Up through v4, we had reports of a bug that could happen when children subscribed before parents. At a technical level, the actual issue was:

  • combining stale ownProps with new state in mapStateToProps()
  • running mapStateToProps() for a component that will be unmounted later in the overall render cycle, combined with a failure to handle cases where the values needed from the store might not exist

As an example, this could happen if:

  • A connected list immediately rendered connected list items
  • The list items subscribed before the parent list
  • The data for a list item was deleted from the store
  • The list item's mapState then ran before the parent had a chance to re-render without that child
  • The mapState tried to read nested state without safely checking to see if that data existed first

v5 specifically introduced an internal Subscription class that caused connected components to update in a tiered approach, so that parents always updated before children. We removed that code in v6, because context updates top-down already, so we didn't need to do it ourselves.

Whatever solutions we come up with should avoid re-introducing this issue.

Allow Shipping Redux Hooks

The React community as a whole is eagerly awaiting the final public release of React Hooks, and the React-Redux community is no exception. Redux users have already created a multitude of unofficial "Redux hooks" implementations, and have expressed a great deal of interest in an official set of hooks-based APIs as part of React-Redux.

We absolutely want to ship our own official Redux hooks as soon as possible. Whatever changes we decide on need to make that feasible.

Potentially Use Hooks in connect

When hooks were announced, I immediately prototyped a proof of concept that reimplemented connect using hooks internally. That simplified the connect implementation dramatically.

I'd love to use hooks inside React-Redux itself, but that would require bumping our peer dependency on React from the current value of 16.4 in v6, to a minimum of 16.8. That would require a corresponding major version bump of React-Redux to v7.

That's a potential option, but I'd prefer not to bump our own major version if we can avoid it. It should be possible to ship a useRedux() as part of our public API as a minor 6.x version, and leave it up to the user to make sure they've got a hooks-capable version of React if they want to import that hook. Then again, it's also possible that a hooks-based version of connect would be necessary to solve the other constraints.

Continue to Work with Other Use Cases

The broader React-Redux ecosystem has updated itself to work with v6. Some libraries have had to change from using the withRef option to forwardRef. Other libraries that were accessing the store out of the (undocumented private) legacy context have switched to accessing the store out of our (still private) ReactReduxContext instance.

This has also brought up other semi-niche use cases that we want to support, including having connect work with React-Hot-Loader, and support for dynamically updating reducers and store state in SSR and code-splitting scenarios.

The React-Redux community has built some great things on top of our baseline capabilities, and we want to allow people to continue to do so even if we don't explicitly support everything ourselves.

Courses of Action

So here's where the rubber meets the road.

At the moment, we don't have specific implementations and solutions for all those constraints. We do have some general outlines for some tasks and courses of action that will hopefully lead us towards some working solutions.

Switch connect Back to Direct Subscriptions

Based on guidance from the React team, the primary thing we should do at this point is switch connect back to using direct subscriptions.

Unfortunately, we can't just copy the v5 implementation directly into the v6 codebase and go with it as-is. Even ignoring the switch from legacy context to new context, v5 relied on running memoized selectors in componentWillReceiveProps to handle changes to incoming props, and then returning false in shouldComponentUpdate if necessary. That caused warnings in <StrictMode>, which we want to avoid.

We need to design a new internal implementation for store subscription handling that satisfies the listed constraints. We've done some early experiments, but don't have any specific approaches yet that we can say are the "right" way to do it.

We do actually already put the store instance into createContext, so nothing needs to change there. The specific values we put into context are not considered part of our public API, so we can safely remove the storeState field from context.

Bringing back direct subscriptions does mean that we can probably bring back the ability to pass store as a prop directly to connected components. That should hopefully resolve the concerns about testing and isolated alternate-store usage, because it's the same API that solved those use cases previously.

Expand Test Suite for More Use Cases

We currently have a fairly extensive unit test suite for connect and <Provider>. Given the discussions and issues we're facing, I think we need to expand that suite to make sure we're better covering the variety of use cases the community has. For example, I'd like to see some tests that do Enzyme shallow rendering of connected components, mock (or actual) SSR and dynamic loading of slice reducers, and hopefully tests or apps that show the actual problems we might face in a Concurrent Mode or Suspense environment.

Consider Marking Current Implementation as Experimental

The v6 implementation does work, and there may be people who prefer to use it. Rather than just throw it away, we could potentially keep it around as a separate entry point, like import {connect, Provider} from "react-redux/experimental".

Improve Benchmarks Suite

Our current benchmarks are somewhat rudimentary:

  • They only measure raw page FPS, no other metrics
  • Measuring FPS requires cranking up the number of connected components and update frequency to arbitrarily high levels until the FPS starts to drop below 60
  • The current benchmark scenarios are sorta meant to represent certain use cases, but are probably not good representations of real-life behavior

I would really like our benchmarks to be improved in several ways:

  • Capture more metrics, like time to mount a component tree, time to complete a single render pass for an entire tree, and breaking down the time spent in different aspects of a single update cycle (running the reducer, notifying subscribers, running mapState functions, queuing updates, wrappers re-rendering, etc).
  • We should see how the benchmarks behave when used with ReactDOM's unstable_batchedUpdates API, to see how much of a difference that makes in overall performance
  • Our benchmarks currently only include web usage. I would like to be have some React Native benchmarks as well.
  • We should have additional benchmark scenarios for more use cases, like apps with connected forms, a large tree of unconnected components with only leaf components being connected, etc.

In general, we need to better capture real-world behavior.

Officially Support Batched React Updates

ReactDOM has long included an unstable_batchedUpdates API. Internally, it uses this to wrap all event handlers, which is why multiple setState() calls in a single event handler get batched into a single update.

Although this API is still labeled as "unstable", the React team has encouraged us to ship an abstraction over this function officially as part of React-Redux. We would likely do this in two ways:

  • Export a function called batch that can be used directly by end users themselves, such as wrapping multiple dispatches in a thunk
  • Export a ReactReduxEnhancer that would wrap dispatches in unstable_batchedUpdates, similar to how tappleby/redux-batched-subscribe works.

It's not yet clear how much this would improve overall performance. However, this may hopefully act as a solution to the "zombie child component" problem. We need to investigate this further, but the React team has suggested that this would be a potential solution.

Currently, ReactDOM and React Native apparently both separately export their own versions of unstable_batchedUpdates, because this is a reconciler-level API. Since React-Redux can be used in either environment, we need to provide some platform abstraction that can determine which environment is being used, and import the method appropriately before re-exporting it. This may be doable with some kind of .native.js file that is picked up by the RN build system. We might also need a fallback in case React-Redux is being used with some other environment.

Hooks

We can't create useRedux() hooks for React-Redux that work correctly with v6's context-based state propagation, because we can't bail out of updates if the store state changed but the mapState results were the same. However, the community has already created numerous third-party hooks that rely on direct store subscriptions, so we know that works in general. So, our ability to ship official hooks is dependent on us first switching back to direct store subscriptions in connect, because both connect and any official hooks implementation need to share the same state propagation approach.

There's a lot of bikeshedding that can be done about the exact form and behavior of the hooks we should ship. The obvious form would be to have a direct equivalent of connect, like useRedux(mapState, mapDispatch). It would also be reasonable to have separate hooks like useMapState() and useMapDispatch(). Given the plethora of existing third-party hooks libs, we can survey those for API and implementation ideas to help determine the exact final APIs we want to ship.

In theory, we ought to be able to ship these hooks as part of a 6.x minor release, without requiring that you have a minimum hooks-capable version of React. That way, users who are still on React <= 16.7 could conceivably use React-Redux 6.x, and it will work fine as long as they don't try to actually use the hooks we export.

Long-term, I'd probably like to rework connect to be implemented using hooks internally, but that does require a minimum peer dependency of React 16.8. That would be a breaking change and require a major version bump for React-Redux. I'd like to avoid that on general principle. But, if it turns out that a hooks-based connect implementation is actually the only real way to satisfy the other constraints, that may turn out to be necessary.

Requests for Community Help

There's a lot of stuff in that list. We need YOUR help to make sure React-Redux works well for everyone!

Here's how you can help:

  • Help us come up with the right approach for using direct subscriptions in connect, including experimenting with implementations yourself, and giving us feedback on any test releases we publish.
  • Improve our benchmarks suite and test suite by describing use cases that aren't currently covered, adding new tests and benchmarks to cover additional scenarios, and updating the benchmarks suite to capture additional metrics
  • We need to determine exactly what limitations React-Redux faces with Concurrent React. The React team has some rough examples of concurrent React usage - porting those to use Redux would help show what specific problems might happen.
  • Implement our support for batched updates, and make sure it works in different environments.
  • Discuss what our future hooks-based APIs should look like, including rounding up the existing third-party libs to help guide the API design.

We can start with some initial discussion in this issue, but I'll probably try to open up some specific issues for these different aspects in the near future to divide up discussion appropriately.

Final Thoughts

There's a lot of great things ahead for React. I want to make sure that React and Redux continue to be a great choice for building applications together, and that Redux users can take advantage of whatever capabilities React offers if at all possible.

The React-Redux community is large, growing, smart, and motivated. I'm looking forward to seeing how we can solve these challenges, together!

@Jessidhia

This comment has been minimized.

Copy link

commented Feb 3, 2019

I'm not really sure the batchedUpdates strategy alone can deal with the update bailout performance issue.

It would allow react-redux to move the bailout to the shouldComponentUpdate phase instead of having connect() return memoized children to skip child reconciliation, but it'd still trigger a render pass on every single connect() HOC and a full React root traversal (even if it is 100% bailouts), so I estimate the cost to only be marginally smaller than v6.0's approach. It would allow for a potential hooks API to bail out, though, which would be a usability gain at least.

I still have some ideas for things to try without breaking API, though.

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

I'm open to experimenting with as many variations on implementation approaches as we can come up with :) All the more reason to have additional tests that can quantify and express the additional constraints we need to meet, beyond what we have in the repo now.

@dschinkel

This comment has been minimized.

Copy link

commented Feb 3, 2019

Bringing back direct subscriptions does mean that we can probably bring back the ability to pass store as a prop directly to connected components. That should hopefully resolve the concerns about testing and isolated alternate-store usage, because it's the same API that solved those use cases previously

Thank you!!

@gaearon

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

FWIW I don’t see anything preventing us from adding provider support to shallow renderer. If that’s the concern about testing.

@gaearon

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

@Jessidhia The batchedUpdates thing is more about fixing the “zombie child”. It’s the idiomatic React solution for always traversing from the top. It can also improve perf outside event handlers but avoiding the bad ordering is the main motivation.

@thomschke

This comment has been minimized.

Copy link

commented Feb 3, 2019

Please don't forget to support immutable data structure libraries like immutable-js.

They have a special equality check like is() and we need a way to define a custom comparison function for connect or useRedux.

see facebook/react#14569 (comment)

@Ephem

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

This is a fantastic writeup, sincerely thank you so much for your hard work @markerikson! 🙌

The specific values we put into context are not considered part of our public API, so we can safely remove the storeState field from context.

I do agree with this. That said, this will technically break some implementations of dynamically injected reducers in v6 since they currently have to rely on those private APIs, so I was glad to see you explicitly mention that issue. 😀 I don't think any of the "big ones" are using storeState for their implementation currently though (at least not dynostore or dynamic-modules I think) so shouldn't be a huge issue.

I really agree with a test-based approach here, if I can find the time I'll try to contribute some tests focusing on SSR.

@vincentjames501

This comment has been minimized.

Copy link

commented Feb 3, 2019

Just wanted to say I'm excited about this roadmap and feel it's the correct direction forward until both context performance for large stores with frequent updates and bailout of useContext is added/supported. I'd love to see a compilation of all the hooks approaches the community has developed thus far and review in a centralized place! Awesome work @markerikson , the community greatly appreciated your work!

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

@vincentjames501 : yup, agreed.

A couple months ago, @adamkleingit put together this list of existing Redux hooks libs:

https://twitter.com/adamklein500/status/1072457324932067329

Collating that list of existing Redux hooks libs is absolutely something that someone else besides me can do, and would really help (like, a Google Docs spreadsheet or something like that). Links, summaries, API comparisons, etc.

Others not in that list as I run across them:

https://github.com/animify/useRestate

@epeli

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

It's really interesting to me that you mention using setState batching as a potential solution because that's the way I originally solved the zombie/tearing issue for react-redux in the #99 PR. I guess things can come around sometimes :)

That PR didn't actually use the unstable_batchedUpdates itself but one could enable it via the redux-batched-updates middleware in order to avoid the zombie child issue.


In the @gaearon's "React as a UI Runtime" article he explains how and why React uses batching for event handlers

https://overreacted.io/react-as-a-ui-runtime/#batching

This really resonates with me because the reason for it seems to be basically same as why Redux would need it.

And using it the issue is pretty easy to solve. Inspired by this I created my own redux-hooks implementation that does not suffer from the zombie issue:

https://github.com/epeli/redux-hooks

It works by using static context which contains the store reference and an array reference. When the useReduxState() is called it appends an update() function into the array by mutating it (context values are never updated in this implementation). Only the <HooksProvider> subscribes to the store. On a store update it just iterates over the update function and calls them inside the unstable_batchedUpdates callback:

https://github.com/epeli/redux-hooks/blob/f26938076f4a253a83fef4823cfbcd5b2f52fc01/src/redux-hooks.tsx#L26-L30

Here's a simple todoapp using this implementation

Codesandbox: https://codesandbox.io/s/github/epeli/typescript-redux-todoapp/tree/hooks
Github: https://github.com/epeli/typescript-redux-todoapp/tree/hooks

I really cannot comment about the performance of this since I just put it together but I don't think there's any reason why it couldn't be good. This implementation now just does the basic performance optimization by bailing out if mapState does not produce new value (shallow equal check).

I'd be super interested hearing any feedback on this. Even just for the internal hooks usage. This is the first thing I've ever written with hooks.


EDIT: Oh, and this not a hooks specific way to implement this. The same method would work for the regular HOC connect(). I just wanted to play with hooks a bit (and I don't think there are any other zombie free implementations of redux hooks out there).

@alexreardon

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

@markerikson ❤️

To weigh in slightly: for react-beautiful-dnd we have not moved to react-redux v6 for performance reasons https://david-dm.org/atlassian/react-beautiful-dnd

Here was my initial performance investigations of the new context api for propigating updates: facebook/react#13739

@solkimicreb

This comment has been minimized.

Copy link

commented Feb 4, 2019

Hi everyone!

I just migrated one of my libs (which has a pretty extensive test coverage) to use unstable_batchedUpdates and I can confirm two things.

  • Exposing two builds - one .js and one .native.js - and adding the .js build as the main to package.json does not work for react-native. Exposing a single build with extra platform chunks (platform.js and platform.native.js) and requiring platform from the main chunk does work for both react-dom and react-native however. You can check the implementation here. (Check the src and the platforms folders and the scripts/build.js file.)

  • unstable_batchedUpdates does solve the zombie child issues. I used to have extra logic to reorder setState calls in a single sync batch to guarantee a parent-child order in the renders. I could totally remove this after I wrapped sync batches with unstable_batchedUpdates which seems to order re-renders in a parent-child order even if the setState calls are in a different order (subscription order I assume).

I hope these help a bit.

@dmytro-lymarenko

This comment has been minimized.

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

I've done some early experimenting the last couple evenings with trying to rework the guts of connect. My utterly hacked-up WIP experimental code is here:

https://github.com/reduxjs/react-redux/tree/connect-subscription-experiments

A few notes:

  • We can't reuse the same subscription init approach we did in v5, because that relied on having access to this.context.store in the constructor. With createContext, there's no way to grab the store reference that early, because you only have access to it in render(). The only options there are to have a wrapper component whose only job is to extract whatever you need from context and pass it to a child component as a prop that does the real work, or use static contextType. I don't think either of those are workable here, because a wrapper adds more levels of nesting (probable perf hit) and v6 allows the user to customize what context instance they use at runtime (so we can't declare/assign that statically).
  • So, just for kicks, I've opted to jump right into experimenting with using hooks directly inside of connect, and writing direct subscription handling logic. I briefly considered trying to write a useRedux() hook first, and then use that in connect, but decided it was better to get connect working right first. We can extract that into a working hook later.
  • My first attempt passed the tests, but did all the derivation in the function component itself. A quick perf benchmark showed it was horribly slow, only 50% the speed of v5 (and slower than v6). Basically, I reinvented new context, badly.
  • My current WIP passes most tests, but the ones it doesn't pass are concerning. I think that batched updates are handling the "zombie child" issue, but in the case of children subscribing first, child props are being recalculated based on existing wrapperProps when the store updates, because the parent hasn't re-rendered itself yet and passed down new props. This makes me think we'd have to bring back something similar to the Subscription class we had in v5 to enforce top-down update behavior for that case.

FWIW, this is just me having hacked on stuff for a couple evenings - I'm just throwing out what I've tried and thought of so far.

@Ephem

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Great work, I will try to dive deeper into it when I can, but I immediately had one question. Since connect is a HOC that returns a new component for each call, and custom context can only be configured in that outer function call, would it not be possible to use static contextType after all? Only way to change that context would be to call connect again to return a new component, so isn’t it static in that sense?

Haven’t checked your code or any details so might well be missing something which is why I’m asking. Might try to play with this tomorrow. :)

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

No, the v6 implementation specifically supports passing a custom context instance as a prop directly to a connected component, regardless of whether or not you also passed a custom context instance as an option when calling connect():

render() {
const ContextToUse =
this.props.context &&
this.props.context.Consumer &&
isContextConsumer(<this.props.context.Consumer />)
? this.props.context
: Context
return (
<ContextToUse.Consumer>
{this.indirectRenderWrappedComponent}
</ContextToUse.Consumer>

This was primarily intended to act as an alternative for the removal of the "store as prop" ability, so that you could have an isolated portion of the subtree reading from a different store than the rest of the app if desired.

@Ephem

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Thanks for explaining, I had somehow missed that part. Certainly makes it trickier. :/

@Ephem

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Using context from props does indeed seem like it necessarily adds another wrapper if not using hooks. I guess one approach to handle it with backwards compat might be to check if context-prop exists, not set up subscription in that case and render extra wrapper only then (+handle prop-change case..). Seems tricky and possibly verbose though.

Good performance in default case or custom context via connect, extra wrapper only if context is defined as prop? Most common case for that is tests as I understood it? Just brainstorming. :)

@epeli

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

Could explain a little or point to a test of this "children first" scenario?

  • I think that batched updates are handling the "zombie child" issue, but in the case of children subscribing first, child props are being recalculated based on existing wrapperProps when the store updates, because the parent hasn't re-rendered itself yet and passed down new props.

I'd like to take a look at it.

@MrWolfZ

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2019

@markerikson I have created a PR that adds a test for the issue with my suggested change I mentioned above.

Also, regarding the use of batching to replace the tiered subscriptions, I have to point out one thing. My whole suggestion depends on the assumption that mapStateToProps should be a pure/side-effect free function. However, I have looked at the official documentation and nowhere does it state that mapStateToProps must be pure. So, if this is not the case, then my argumentation is moot.

If mapStateToProps is indeed required to be pure, then I believe what you are trying to achieve (and what your tests check for) - namely that mapStateToProps is never called with an inconsistent combination of ownProps and store state - is not what needs to be ensured. Instead, the correct invariant to keep should be that a render is never executed with the result of a mapStateToProps call with an inconsistent combination of ownProps and store state.

So my question is this: is mapStateToProps intended to be a pure function?

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

@MrWolfZ : yes, mapState should always be pure, the same way reducers and other selectors should be pure. We can't catch and enforce that they're pure, but they should be. (If we don't explicitly say that in the docs, we ought to.)

The early issues that I linked around stale props included cases where mapState functions were trying to read values from the store that no longer existed, because the data had been removed. Based on that store state, the component should have been un-rendered already, but the bottom-up firing of subscriptions was breaking that assumption.

I suppose you can argue that users should write more defensive mapState functions to avoid throwing errors, but our goal has always been that mapState would be called with consistent props and store state.

@MrWolfZ

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2019

@markerikson In that case I think my suggestion with batched updates would still work, but would require some changes to the current error handling and the way mapState is called. As you mentioned in this comment, using batched updates does not prevent stale props from being passed to mapState. I argue that this does not matter if we change the error handling as follows.

Currently, errors thrown during the execution of mapState in the subscription callback get caught and re-thrown during render. However, this re-throwing should not be required, since during render mapState will be executed again, and due to it being a pure function, if it is called with the same parameters that caused the error, it will throw again, just as if we had thrown the error ourselves. If the parameters changed however (e.g. because props have been updated to be consistent with the state) then it won't throw. Also, in case the component got unmounted, the render won't be called at all, so no issue there. So in your scenario it would work like this:

  • a store update occurs that removes the last list item
  • the subscription callback for the last list item will be executed and the call to mapState will fail, due to the inconsistent state and props, but since the error is caught nothing happens
  • the subscription callback of the parent is executed
  • all render changes are applied together due to batching and the child never gets rendered due to the parent rendering first and the list item being removed

It would also work for cases where the element is still in the component tree. However, this change would require to always call mapState during render, since the result from the subscription callback might have been created with inconsistent props and state.

In summary, this way we may be able to make use of batched updates, but even if it works (and I am sure I have missed and edge case or two) it would come at the cost of performance due to having to call mapState always during render (although you are already memoizing it, so it shouldn't be that bad). Still, I think the current implementation works just fine, so no reason to change it. However, when it comes to hooks it may be worthwhile to revisit this idea, since it obviates the need for a HOC.

PS: While evaluating this idea, I found an issue with the way mapState calls are memoized. I created a separate issue for this.

EDIT: Also, it should of course be noted that the batching would only work on platforms where it is actually available, so currently wouldn't work with react native, as that doesn't have support for batching if I understand correctly. A (probably stupid) alternative would be to simply call the subscription callbacks in reverse order, but I have no idea if this can work.

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

@MrWolfZ : I do appreciate you spending a bunch of time digging into this and discussing ideas. However, at this point, I'm not seeing a pressing need to go making drastic changes to what appears to be a working v7 beta implementation.

If you feel that a different approach would work better, I'd encourage you to take the time to implement it and file a PR for discussion. The key criteria are:

  • Must either pass all existing tests either as-is, or have specific justification for why a test was changed based on the implementation differences before and after (ie, number of mapState calls used as an expectation, and "in vPrev it would call 3 times because X,Y,Z, and in vNew it calls 4 times because...")
  • Must be at least as fast as v5 in our benchmarks suite

The release of v7.0 isn't a once-and-done thing for the development of that line. Obviously we hope to release a 7.x later with a hooks API. If there's alterations to the implementation that improve things, we can certainly incorporate those and release a minor version.

Also, batched updates definitely exist in RN's renderer - we're importing that method when built in an RN environment.

So, not trying to cut off this discussion completely, but at this point my view is that v7 is basically code-complete except for whatever bugfixes come up. If you or anyone else can show that there's distinct improvements we can make before release, I'm entirely open to evaluating those, but I don't want to burn time on hypotheticals right now. Show me code :)

@MrWolfZ

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2019

@markerikson Yeah, as I wrote in my post, I don't think anything should be changed for v7.0 since it works just fine. I was thinking more about the future, especially when it comes to hooks. It seems you want to keep this issue about v7.0, so I guess I'll better take my thoughts (and soon hopefully some code) to the appropriate issue then.

@jeremygottfried

This comment has been minimized.

Copy link

commented Mar 25, 2019

@markerikson thank you for the previous response, as that cleared up a lot for me! Looking at the new batch update API, I am curious why you expose that directly? It seems like update notifications are batched by default in the provider, so what are some possible use cases for using the unstable_batchedUpdates directly when I dispatch actions?

Your readme shows this as an example:

batch(() => {
            dispatch(increment());
            dispatch(increment());
        })

But it seems like that example is already being handled when the Provider or a subscription calls the
notifyNestedSubs method. Am I wrong about that?

@saboya

This comment has been minimized.

Copy link

commented Mar 25, 2019

What I understand is that when you wrap two dispatches with a batch, it will only update the state once, as far as subscribers go.

@jeremygottfried

This comment has been minimized.

Copy link

commented Mar 25, 2019

@saboya I think I understand now. If you have multiple promises resolving close to each other, you can use batch so that many dispatches only result in a single render. This is different use case from connect and provider, where batch is preventing unnecessary renders within nested subscriptions.

React's unstable_batchedUpdate() API allows any React updates in an event loop tick to be batched together into a single render pass.

@alexreardon

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

Is there anything left outstanding for v7? I have been using the beta in react-beautiful-dnd for a few days and I have not found any issues

@wood1986

This comment has been minimized.

Copy link

commented Apr 3, 2019

In v6, every Redux state update immediately causes a setState() in at the root of the tree, and React always has to walk through the component tree to find any connected components that may be interested in the new state. This means that v6 ultimately results in more work being done for each Redux store update. For usage scenarios with frequent Redux store updates, this could result in potential slowdowns.

@markerikson Have you thought about do something which is similar to the ConnectComponent? This can aviod going each connected component.

import React, {
  memo,
  useCallback,
  useContext,
  useEffect,
  useRef,
  useState
} from "react";

const StoreContext = React.createContext({}, () => 0), // avoid going thought each connected components and let subscribe to handle when to re-render
  ConnectComponent = memo(props => {
    const [state, setState] = useState(null),
      { subscribe, unsubscribe } = useContext(StoreContext);

    useEffect(
      () => {
        subscribe(props, setState); // this passes the setState to subscribe to deteremine when to re-render
        return () => {
          unsubscribe(props, setState);
        };
      },
      [props]
    );

    return React.Children.map(props.children, child =>
      React.cloneElement(child, { ...props, ...state })
    );
  }),
  useStore = (initialState = {}, keyFunc, actionFunc) => { // Assume this is a key value pair store
    const store = useRef(new Map(Object.entries(initialState))),
      listenersRef = useRef(new Map()),
      subscribe = useCallback((props, setState) => {
        const key = keyFunc(props);

        if (store.current.has(key)) {
          setState(store.current.get(key));
          return;
        }

        const isResolving = !listenersRef.current.has(key);

        listenersRef.current = new Map(listenersRef.current);
        const listeners = new Set(listenersRef.current.get(key));
        listeners.add(setState);
        listenersRef.current.set(key, listeners);

        if (!isResolving) {
          let promise = actionFunc(props);

          if (!(promise instanceof Promise)) {
            promise = Promise.resolve(promise);
          }

          promise.then(state => {
            store.current = new Map(store.current);
            store.current.set(key, state);
            listenersRef.current.get(key).forEach(setState => setState(state));
            listenersRef.current = new Map(listenersRef.current);
            listenersRef.current.delete(key);
          });
        }
      }, []),
      unsubscribe = useCallback((props, setState) => {
        const key = keyFunc(props);
        let listeners = listenersRef.current.get(key);
        if (listeners) {
          listenersRef.current = new Map(listenersRef.current);
          listeners = new Set(listenersRef.current.get(key));
          listeners.delete(setState);
          if (listeners.size === 0) {
            listenersRef.current.delete(key);
          } else {
            listenersRef.current.set(key, listeners);
          }
        }
      }, []);

    return [subscribe, unsubscribe];
  },
  StoreContextProvider = props => {
    const [subscribe, unsubscribe] = useStore(
      props.configs,
      (props) => props.id,
      (props) => fetch(props.url).then((res) => res.json())
    ); // eslint-disable-line react/prop-types

    return (
      <StoreContext.Provider value={{ subscribe, unsubscribe }}>
        {props.children}
      </StoreContext.Provider>
    );
  };
@IsenrichO

This comment has been minimized.

Copy link

commented Apr 4, 2019

@wood1986 Can you explain this line:

const StoreContext = React.createContext({}, () => 0);

I can't seem to find any mention of the the createContext method taking a second parameter in React's docs.

@Jessidhia

This comment has been minimized.

Copy link

commented Apr 4, 2019

It's unstable / intentionally undocumented, but implementation-wise, always returning 0 there means that no context updates ever directly propagate to any consumer. This blog post goes over it, but you can't specify observedBits on useContext for example.

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

@IsenrichO : yeah, it's undocumented. See my writeup here:

#1018: Investigate use of context + observedBits for performance

@IsenrichO

This comment has been minimized.

Copy link

commented Apr 4, 2019

Noted. Thank you all for the info!

@dai-shi

This comment has been minimized.

Copy link

commented Apr 4, 2019

but you can't specify observedBits on useContext for example.

@Jessidhia Do you mean you can't specify observedBits in the second argument of useContext? I specify it.
https://github.com/dai-shi/react-hooks-global-state/blob/1edf110120b7c3019db51989bf72ef3891f9d23d/src/index.js#L77
(this one does a tricky thing to hide warning, though. It was originally like this.)

@wood1986

This comment has been minimized.

Copy link

commented Apr 4, 2019

@Jessidhia @dai-shi I tested observedBits did work in Hooks. Otherwise, I won't suggest this.

@Jessidhia

This comment has been minimized.

Copy link

commented Apr 4, 2019

I see that it's now forwarded to readContext; it was broken at some point (with a warning, even) during the 16.8 cycle, likely pre-release.

@julienben

This comment has been minimized.

Copy link

commented Apr 7, 2019

@markerikson I tested the latest beta in react-boilerplate and several work projects. No issues to report!

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

Folks, it's time to finally bring this issue to a close.

Because I've just published React-Redux v7 final!!!!!

Go install it :)

Nice to know that this is done and there's no other long, contentious, complex API design threads to figure out after this.

oh waitaminute

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.