options.arePropsEqual for custom prop equality comparison

[tgriesser]

Adds new arePropsEqual option, defaulting to shallowEqual if not provided.

Docs & test included!

[acdlite]

What's the value proposition here? What comparison function besides shallowEqual would a person reasonably use?

[gaearon]

@acdlite I believe it's about enabling per-component memoization: #179, #180, #182

[acdlite]

Oh okay that makes sense to me. So this is basically React Redux's version of shouldComponentEqual but instead of preventing re-renders it's preventing Connect from re-computing state selection and action creator props (which in turn helps prevent re-renders, in addition to firing selectors and re-binding action creators).

Could we rename the option to be more precise? The name arePropsEqual gives the impression that it may be used to implement shouldComponentUpdate. Maybe shouldConnectUpdate?

[tgriesser]

I think we've concluded it's actually not possible to do the per-component memoization without a global LRU cache as @ellbee mentions here, so this would be the next best thing.

@acdlite I typically like to use a one layer deeper shallow ===. I have a lot of connected components and selectors which have plain Arrays as return values, and so it ends up there's a lot more unnecessary updating going on than there'd need to be if we could just check the returned Array or Object props are themselves shallow equal.

I've found the amount of time it takes to do an extra shallow compare of a few Arrays is virtually nothing compared to the expense of React going through an update cycle.

[acdlite]

I've found the amount of time it takes to do an extra shallow compare of a few Arrays is virtually nothing compared to the expense of React going through an update cycle.

If we're optimizing render cycles, that can be addressed using shouldComponentUpdate on the connected component, right?

If we're optimizing selectors / action creator binding, then this option makes sense.

[tgriesser]

It's sort of both, but yes if the connected component is receiving props containing Object/Array values, I'd imagine depending on the use case it too could benefit from doing a slightly more sophisticated arePropsEqual to avoid unnecessary updateStateProps / updateDispatchProps.

[ellbee]

This was probably your editor being helpful, but the trailing whitespace here was to create a line break in the rendered markdown, and should be put back.

[ellbee]

This looks reasonable to me.

[tgriesser]

@ellbee updated, although before we go with this (although I do think it'd be a nice option to configure), I've got one more idea on an approach for #179 that wouldn't be too intrusive.

[ellbee]

@tgriesser Appreciate the thoroughness of trying out all these approaches! I wish I had some time right now to look at this and give some thoughts, but something's come up and I'm not going to have any 'github time' at all in the next few days 😞

[tgriesser]

@ellbee no worries / no rush, whenever you get around to it! I've had the same problem with my own projects lately, have no problem working off of a fork.

I really appreciate a project which takes longer to figure out the simplest solutions rather than to just merge everything - keep up the great work all! 👍

[ellbee]

@tgriesser Assuming we can think of an acceptable name for the option I think I like this PR the best; it doesn't add to the top level API and changes to existing code are minimal.

Coming at it from the Reselect angle, would a selector like the following a) work b) be performant for your use case? It occurs to me that in the common case there is probably a prop (or a few props combined) that will uniquely identify an instance of the component and can therefore be used as a key for the cache (I suppose a key could be purposely added to the props if this is not the case). I have been benchmarking a few different things and this one seems pretty good.

import LRU from 'lru-cache'

function defaultEqualityCheck(a, b) {
  return a === b
}

export default function memoizeWithCache(
  func,
  hashFn,
  cacheOptions = 500,
  eq = defaultEqualityCheck 
) {
  let cache = LRU(cacheOptions)
  return (...args) => {
    const key = hashFn(args)
    let result = cache.get(key)
    if (result && args.every((arg, i) => eq(arg, result.args[i]))) {
      return result.result
    }
    result = func(...args)
    cache.set(key, { result, args })
    return result
  }
}

import memoizeWithCache from './memoizeWithCache'
import { createSelectorCreator } from 'reselect'
import shallowEquals from 'is-equal-shallow'

// take the second argument and use as the hash key
const hashFn = args => args[1]

const createSelectorWithCache = createSelectorCreator(
  memoizeWithCache,
  hashFn,
  500,
  shallowEquals
)

// each row in state.table will be rendered with a separate connect
const selector = createSelectorWithCache(
  (state, props) => state.table[props.rowId],
  // pass the row into the result fn to use as the cache key
  (state, props) => props.rowId,
  // key is not used in resultFunc, but it is getting picked out by hashFn to use as the cache key
  (rowData, key) => { 
    return {values: rowData} 
  }
)

The hashFn and ignored parameter in the example is hacky but if this does actually turn out to be a usable solution I will do something with Reselect to support this in a nicer way (probably somehow passing the props directly to the hashFn).

[gaearon]

It occurs to me that in the common case there is probably a prop (or a few props combined) that will uniquely identify an instance of the component and can therefore be used as a key for the cache

👍

[tgriesser]

@tgriesser Assuming we can think of an acceptable name for the option I think I like this PR the best; it doesn't add to the top level API and changes to existing code are minimal.

Ah cool, thanks for sharing this does look pretty neat - I'll need to dig in a bit and see how this ends up working in practice.

At first glance I still believe it requires you to know too much about the shape of the component props you're connecting to in order to properly use the cache. If a key is required to make guarantees it seems it'd be leaking the selector implementation details into the component. While it probably works well it, my initial impression is that it's the wrong place to be solving the issue of having every connected component instance share a singleton selector.

Top level API concerns aside (and I'd contend it's not much of an addition since it's really a higher order connect - analogous to createSelectorCreator in Reselect) did you get a chance to look at what all #185 does? It passes connect through connectComponent, providing an outlet for solving the underlying issue with no change to the current API use.

[ellbee]

At first glance I still believe it requires you to know too much about the shape of the component props you're connecting to in order to properly use the cache.

Yeah, you do have to specify how to pick out the "cache key" from the props, but you already have to know the shape of the props to write the selector. And if you aren't using props in the selector then you don't have the problem in the first place.

If a key is required to make guarantees it seems it'd be leaking the selector implementation details into the component.

I am struggling to think of a practical situation where you would have to add a dedicated "cache key" prop, I think that that an existing prop (or some combination of existing props) should be enough to define the key, although this is probably a failure of imagination on my part.

While it probably works well it, my initial impression is that it's the wrong place to be solving the issue of having every connected component instance share a singleton selector.

I don't see a problem with a singleton selector that has a cache so I think our instincts differ on this (although I concede that maintaining a selector library means I am probably a little biased to the selector based solution so I am happy to hear arguments that may persuade me otherwise 😄). One scenario in which the caching selector may be preferable is if there are 10 components with one set of props, and 10 components with another set of props. In this case the selector computation would run twice for every state update rather than the 20 times it would run if selectors are being created for each component.

Top level API concerns aside (and I'd contend it's not much of an addition since it's really a higher order connect - analogous to createSelectorCreator in Reselect) did you get a chance to look at what all #185 does? It passes connect through connectComponent, providing an outlet for solving the underlying issue with no change to the current API use.

The thing I am unsure about with #185 is whether it is useful enough outside of this (possibly niche) use-case to warrant an addition to the top level API. It is also a bigger change (in terms of lines of code) than this PR, and in my opinion makes the code a little harder to understand. On the plus side it is probably easier to explain from a documentation point of view. Can a case be made that it is also more general and therefore enables more use-cases than this PR? It seems like it should but I can't think of examples.

Is #185 your preference?

[tgriesser]

Yeah, you do have to specify how to pick out the "cache key" from the props, but you already have to know the shape of the props to write the selector. And if you aren't using props in the selector then you don't have the problem in the first place.

So the thing is, I'm actually creating selectors in a pureRender HOC (so I don't need to re-generate a "factory" selector), and passing the selectors as props, so they're already sort of specific to that component (interested in any feedback about this idea).

@connect(createStructuredSelector({
   value: (state, props) => props.__valueSelector(state),
   field: (state, props) => props.__fieldSelector(state)
}))
class SomeComponent extends React.Component {
  // ...
}

@pureRender
export default class SomePureComponent extends React.Component {
  render() {
    return (
       <SomeClass 
         __valSelector={valueSelector(props.valueId)} 
         __fieldSelector={fieldSelector(props.fieldId)} />
    )
  }
}

I'm sure the LRU cache could be configured to work here, it'd just be a lot uglier.

The issue is that when different prop selectors are used against the same structured selector, it invalidates the final structured selector's memoization pretty much every time. I'm using a single decorator which combines connect and createStructuredSelector with the approach from #185, just because it's very convenient to express:

@connectSelector({
   value: (state, props) => props.__valueSelector(state),
   field: (state, props) => props.__fieldSelector(state)
}))
class SomeClass extends React.Component {
  // ...
}

I think #185 would be my preference, just because it solves the problem more generally in one place, as opposed to going through the ~150 or so selectors.js and thinking about the cache for each. I also don't want to rush any decisions here and would totally be fine with just discussing more / thinking on it for a bit.

Also might be worth getting some benchmarks to make sure that any changes aren't major regressions for common cases... I could take a shot at that in a week or two once I have a bit of extra time.

[ellbee]

Ah, thanks for writing that up. Interesting to see your use case. I'll take a closer look over the weekend when I get some free time.

I think benchmarks would be cool if you have the time.

[compulim]

(I am coming from similar issue in reselect)

Let me do a recap. In short, all we want is support per-instance memoize function. I think there are few things we could do:

• Per-instance memoize function (cache bound to the instance, not "global")
• Per-instance selector function (since memoize function's lifecycle is bound to the selector, i.e. every selector will have their own memoize)
• LRU cache, IMHO, housekeeping and hashing takes too much effort

Since react-redux is designed not to expose the component instance (e.g. when mapStateWithProps is called, this is undefined, and, stateProps returned from mapStateWithProps will not come back again as ownProps), it become very difficult to maintain a per-instance selector/memoize.

@tgriesser suggested we could hold something per-instance by either:

• Keep selector as a props (search __valueSelector in this thread), but it requires parent component to understand what data to "bind", or we need to create another wrapper class
• connectComponent, attaching prop configuration to component #185 "Thunk" approach, connectComponent create the actual selector when the component is created

I like #185 "thunk" approach more because "props" approach requires another wrapper or knowledge in parent. "Thunk" approach is cleaner.

Consider the following nested selector scenario, quotationSelector is a selector shared amongst few pages. And welcomePageSelector is a selector only used by the welcome page.

In the old days, we write something like this, and this doesn't works well with multiple instances:

const quotationSelector = createSelector(
  [
    state => state.quotations,
    (state, props) => props.quotationID
  ],
  (quotations, quotationID) => ({
    quotation: quotations[quotationID]
  })
);

const welcomePageSelector = createSelector(
  [
    state => state.profile,
    quotationSelector
  ],
  (profile, quotation) => ({ profile, quotation })
);

connect(welcomePageSelector)(React.createClass(...));

This doesn't works very well if there are multiple components on the page and each have different props. The memoize function inside createSelector will always cache miss.

Then in the new days (with "thunk" approach):

const quotationSelectorFactory = () => createSelector(
  [
    state => state.quotations,
    (state, props) => props.quotationID
  ],
  (quotations, quotationID) => ({
    quotation: quotations[quotationID]
  })
);

const welcomePageSelectorFactory = () => {
  const quotationSelector = quotationSelectorFactory();

  return createSelector(
    [
      state => state.profile,
      quotationSelector
    ],
    (profile, quotation) => ({ profile, quotation })
  );
};

connectComponent(welcomePageSelectorFactory)(React.createClass(...));

Note that quotationSelector is created inside welcomePageSelectorFactory. Thus, the top-level selector maintains lifecycle of nested selectors (and memoize functions).

I am almost good with #185, except that the name connectComponent and connect is a bit confusing. I have no objection if we are going to obsolete connect.

Update:

In short, I think what we are doing is:

• In the old days, ComponentClass holds a reference to selectorInstance, so every componentInstance share the same selectorInstance
• What we really want, ComponentClass holds a reference to SelectorClass, so every componentInstance could have their own selectorInstance (if they really want, they could use singleton pattern to share the instance)

And we are finding ways, either:

• selectorInstance could identify independent componentInstance, which seems impossible for now
• Every componentInstance will create their own selectorInstance, which connectComponent, attaching prop configuration to component #185 is doing

[tgriesser]

Yep, @compulim gave a great summary of the value proposition of this API addition, and why I believe #185 is correct solution to proper memoization per component instance without needing to alter the existing connect implementation. I just updated against the current master, all tests passing.

I am almost good with #185, except that the name connectComponent and connect is a bit confusing.

Totally. I am up for changing this, and wanted to see what anyone else thought might be a good name. I initially had connectFactory, maybe that'd be better. Or perhaps createConnector? Anyone have any other suggestions?

I have no objection if we are going to obsolete connect.

I don't think changing the current connect implementation is the right option here, since it's a very common approach everyone uses, in the case where you're using both props and state and want to properly memoize, I think exposing this additional top level function is the right approach.

[oyeanuj]

@tgriesser, thanks so much for your work on this!

I don't think changing the current connect implementation is the right option here, since it's a very common approach everyone uses

1. Should memoization per component instance not be the default way of implementing or using connect? What would be good reasons to use the default connect method instead?
2. As we go ahead with this, it would be very helpful to have an example in the README that explains how this works and ways to use this.

[compulim]

@oyeanuj, for your question.

1. Should memoization per component instance not be the default way of implementing or using connect? What would be good reasons to use the default connect method instead?

I think we should not break existing usages. But I do agree the current implementation is weak.

@tgriesser, I think connectFactory is better. But let's wait for @ellbee to make that final call.

Your code is good, I really hope to see it in the official branch sooner.

[oyeanuj]

+1 to getting this merged soon! @ellbee? :)

@tgriesser What documentation do you think we need in the guide/docs for this?

[ellbee]

@oyeanuj Yeah, sorry for not being responsive on this, been super busy 😦 I am going to make some time in the next couple of days to get to all my loose ends.

[markerikson]

Any updates on this? I've got a TON of uses for per-component memoization, and would love to see this become an option.

[gaearon]

I'm happy to get this in, but it doesn't merge cleanly.

[compulim]

@tgriesser do you need some help to bring this branch up-to-date?

[ellbee]

@compulim I am happy to clean this up for this merging if this is the PR we want to use but there is also #185..

@gaearon #185 is another proposal for solving this issue, and although the discussion is happening on this PR, #185 seems to be the preferred proposal of the participants in this thread. #185 is not a breaking change, but it does introduce a new "connect factory" function to the API, while this PR only adds an option to connect. Is adding a new function to the API a deal breaker for #185?

Both PRs look fine to me, so I don't have a big preference.

[gaearon]

Does #185 (comment) make any sense?

[gaearon]

Closing in favor of #185 (comment), assuming I'm not mistaken in that it would work.