Add stateThunk / dispatchThunk options

[tgriesser]

Adds two new options, stateThunk and dispatchThunk. When set to true, it signifies that the value of mapStateToProps or mapDispatchToProps are thunks, called in the constructor & return values stored on the connected component class.

The use case comes from reselect - I noticed that although functions are memoized, when using props it destroys the utility of the memoziation feature. The idea here is that each connected component would return a unique memoized selector, rather than sharing one between all instances of a component and ending up with a ton of cache misses.

So instead of:

@connect(createSelector(...fns))

You'd do:

@connect(() => createSelector(...fns), null, null, {stateThunk: true})

[gaearon]

Can you please elaborate on usage? I don't quite understand this.
@ellbee Is there a way to solve this problem without changing React Redux API? I don't want it to “know” about the fact that user has Reselect, this feels wrong.

[tgriesser]

It isn't necessarily specific to Reselect.

The issue is that in order to properly memoize the mapStateToProps call, the selector needs to have knowledge of the component it's connected to, otherwise the same function is used for all instances of the component and it can be extremely inefficient if the selector uses props.

This seemed like the best way to go about it, without exposing too much information about the component to the mapStateToProps call. In an application where I have ~300-400 connected components it's cut update time from ~20ms to less than 1.

[gaearon]

OK, I get the use case now. Can this be solved as a wrapper around connect? Or by any other means than adding an option? Options are the last resort. Even more so if they change existing argument semantics.

[ellbee]

A wrapper around connect is my initial thought here too. I'm going to give it a try and will report back.

[gaearon]

@ellbee Appreciate you taking a look. Thanks. 💯

[tgriesser]

Can this be solved as a wrapper around connect? Or by any other means than adding an option?

Yep, latest commit just adds an assignMapFunctions method on the component:

assignMapFunctions() {
  this.finalMapStateToProps = finalMapStateToProps
  this.finalMapDispatchToProps = finalMapDispatchToProps
}

This eliminates the need for any additional options and allows the user to wrap this method on the component and wrap/re-assign the mapStateToProps and mapDispatchToProps functions as desired.

Let me know if this seems like a reasonable approach, and if so I can cleanup the commits.

[gaearon]

I don't understand your latest proposal yet and I'm busy with something else, so I trust @ellbee to make the right call on this.

[ellbee]

I have checked out the PR, and the approach looks reasonable but I am concerned that the memoize HOC in the tests is accessing private react-redux API, and I can't currently see how it would work if you don't access private API.

I was also wondering if we can achieve the same thing without making any changes to react-redux? I knocked together the following HOC to be called in place of connect that seems to work for a simple test project:

import React from 'react'
import { connect } from 'react-redux'

export default function connectWrapper(
  Component,
  mapStateThunk,
  mapDispatchThunk,
  mergeProps,
  options = {}
) {
  const Wrapped = class extends React.Component {
    constructor() {
      super()
      this.component = connect(
        mapStateThunk(),
        mapDispatchThunk(),
        mergeProps,
        options
      )(Component)
    }
    render() {
      return <this.component {...this.props} {...this.state} />
    }
  }
  Wrapped.displayName = `WrappedConnect(${Component.displayName})`
  return Wrapped
};

Advantages of the approach above: No changes to react-redux needed.
Disadvantages: Creates a new wrapper class for each instantiation of the component

Advantages of the PR: Does not create a new wrapper class for each instantiation of the component
Disadvantages: Changes to react-redux needed. Still requires a wrapping HOC to be written. Looks like the wrapping HOC needs to use non-public internal react-redux API.

@tgriesser What do you think? Is creating a new wrapper class every time going to be a problem? Have I missed anything?

[gaearon]

Creating a new class is definitely a big problem: React will bail out of reconciling such components. So you won't be able to handle componentWillReceiveProps because every instance is of a different class.

[ellbee]

Blurgh, yes of course, it is not even going to try if they are of different types. Ok, back to the drawing board.

[tgriesser]

I have checked out the PR, and the approach looks reasonable but I am concerned that the memoize HOC in the tests is accessing private react-redux API, and I can't currently see how it would work if you don't access private API.

Yeah it's sort of a tough one... I don't think there's a way to achieve it without changing something about the connect implementation - it's really more of a matter of what's cleanest / least invasive. I've got another approach that might be a little better I'll open a separate PR for.

[tgriesser]

Closing in favor of #180

[tgriesser]

So after thinking about it for a bit, I wouldn't mind if the answer were that this behavior is a non-goal for react-redux.

I could fork and create a react-redux-memoized or something which assumes mapStateToProps and mapDispatchToProps are thunks and leave the api here alone.

[ellbee]

I agree with you, I can't see a way to do this without making some changes to connect.

Just out of interest, is there a reason why using Reselect's createSelectorCreator and a memoization function with a larger cache isn't feasible for you here?

[tgriesser]

I'm actually using createSelectorCreator to provide a shallow equal comparison for object / arrays one level deep which has worked well, but for the application i'm working on the cache would grow really big really fast if there was no way to clear it when a component unmounts and also I'd assume it'd also be impossible to do checks if you had no idea what component you were trying to check props against, without running into stale cache issues.

[ellbee]

Ah, ok. It is useful to know that createSelectorCreator didn't work for you. I am thinking about trying to find (or create) a performant memoization function with a bounded cache that would be useful in this scenario. Allowing createSelectorCreator to take a custom memoize function was supposed to be flexible enough that it could deal with these kinds of scenarios, but I suppose it doesn't really deal with them at all and makes it the end users problem. I think it would be nice if Reselect could be a bit more helpful here and maybe offer some different memoization strategies for different scenarios.

I'd assume it'd also be impossible to do checks if you had no idea what component you were trying to check props against, without running into stale cache issues.

Interesting. Could you explain this point a bit more please? I'm not sure I follow why I would need to know what component I am checking props against.

I could fork and create a react-redux-memoized or something which assumes mapStateToProps and mapDispatchToProps are thunks and leave the api here alone.

This may be the best option if we can't find a slightly cleaner way to get this in. I'm not keen on encouraging the use of the private API. Even if we leave it undocumented, people might start using it if it can be found in the tests.

[tgriesser]

Interesting. Could you explain this point a bit more please? I'm not sure I follow why I would need to know what component I am checking props against.

Once you move beyond a single prop or combo of props it becomes ugly and quite expensive to need to serialize props to use as a cache key. It seems it'd be simpler if it could be more like react's shouldComponentUpdate, where you just locally have current props and next props and can figure it out from there.

Unless I'm missing something obvious (which I might be), I don't think it's something that can be resolved without changing the design of react-redux quite a bit.

Unrelated, but I was curious what the reasoning for the undocumented recomputations function in reselect... was it for testing purposes only or was there some other purpose I was missing?

[ellbee]

From the point of view of Reselect, I was thinking about something along the lines of how Immutable-js allows objects to be a key in a Map (it basically tags the object with a non-enumerable id) to avoid serialization of large objects, combined with something like a LRU cache to avoid the cache size getting out of control. It may come to nothing, but I am going to give it a go.

Yeah, recomputations is purely there for testing.

[tgriesser]

combined with something like a LRU cache to avoid the cache size getting out of control. It may come to nothing, but I am going to give it a go.

Ah okay so that actually may work fine. I took another look at the code and I'm realizing what I'm actually after is a way to configure the shallowEqual comparison in react-redux similar to what is possible in reselect. Just opened #182 to discuss.

[gaearon]

FYI I closed #185 because I don’t plan to merge it as is but I’m happy to consider merging #185 (comment) which should solve the same problem if somebody gets to implementing it.

[gaearon]

This is implemented in #279 and shipped in 4.3.0.
Thanks to everyone!

[ellbee]

🎉 Nice work @tgriesser!