Reselect: Best practices regarding passing props object from mapStateToProps as the only argument to selectors [docs clarification]

[jordanmkoncz]

In my React/Redux projects I'm using reselect extensively for getting data from my store to provide to React components via the connect function of react-redux. In my mapStateToProps function, when calling a reselect selector, I've been just passing the state and the component's props object as arguments to the selector. So far this has worked correctly in terms of functionality, but I'm unsure whether this is a recommended approach and whether there are any downsides to doing this (e.g. performance losses).

I'll provide some basic sample code to try and make it completely clear how I'm using reselect. Note that I'm also using re-reselect in my project so that I don't need to use selector factory functions and also don't need to use a makeMapStateToProps factory function; I'm unsure of how much of a consequence re-reselect would have in terms of how I'm passing the props object as the only argument to my selectors.

App.js (the root component)

import React, { Component } from 'react';
import { View } from 'react-native';
import EventListItem from '../EventListItem';

class App extends Component {
  render() {
    // Assume that activeUserId actually comes from somewhere else in the app, and its value changes reasonably often.
    const activeUserId = 'baz';

    return (
      <View>
        <EventListItem eventId="foo" activeUserId={activeUserId} />
        <EventListItem eventId="bar" activeUserId={activeUserId} />
      </View>
    );
  }
}

export default App;

EventListItem.js

import React, { PureComponent } from 'react';
import { Text } from 'react-native';
import PropTypes from 'prop-types';
import { connect } from 'react-redux';
import * as selectors from '../selectors';

class EventListItem extends PureComponent {
  static propTypes = {
    eventId: PropTypes.string.isRequired,
    eventName: PropTypes.string.isRequired,
    activeUserId: PropTypes.string.isRequired,
  };

  render() {
    const { eventName, activeUserId } = this.props;

    return <Text>{eventName} - {activeUserId}</Text>;
  }
}

EventListItem = connect((state, props) => ({
  eventName: selectors.selectedEventName(state, props),
}))(EventListItem);

export default EventListItem;

selectors.js

import createCachedSelector from 're-reselect';

// state.entities contains my normalized entities, so state.entities.events is an object where each key is the ID of an event, and each value is the event object itself.
export const allEvents = state => state.entities.events;

// Rather than directly passing eventId as an argument, I'm just passing the whole props object and then pulling out the eventId from this.
export const selectedEventId = (state, props) => props.eventId;

export const selectedEvent = createCachedSelector([selectedEventId, allEvents], (eventId, events) => events[eventId])(
  selectedEventId
);

export const selectedEventName = createCachedSelector(
  [selectedEvent],
  event => event.name
)(selectedEventId);

Hopefully this example is fairly straightforward. Assume that my Redux store already has all of the data it needs (i.e. state.entities.events is already populated with all of my events).

Now where my concern comes in is regarding the impact of a prop like the activeUserId prop that EventListItem takes. For this example, assume that some other part of the app (not shown in the example code) controls the value of activeUserId and that its value changes reasonably often. As you can see in the example code, the value of activeUserId is completely irrelevant to my selectors, the only thing they care about is the eventId prop. However, it is my understanding that because reselect will recompute the value of a selector every time any of its inputs changes, this means that every time activeUserId changes, my selectedEventName selector will be called and its value will be recomputed (rather than returning the cached value that was previously computed) even though in reality this is completely unnecessary since props.eventId has not changed, and the new computed value will be exactly the same as the previously computed value.

Am I correct in my understanding of how this would work? If so, it seems that passing the props object to my selectors like this would result in some performance loss, since selectors are being recomputed unnecessarily. But perhaps this performance loss is negligible/inconsequential, and this would only be a concern if the resolver function for the selectedEventName selector was doing some expensive computation (rather than simply returning a property of an object).

The reason I'm using this approach in the first place is because of the flexibility it provides in composing selectors. If I have multiple selectors that care about different arguments, I don't ever have to worry about what arguments I pass to my selectors or the order of these arguments and ensuring that different selectors are compatible with each other with regards to the arguments they take. Instead, I can always just pass an object containing various different props as an argument to my selectors and they can then extract the props they care about, e.g. one uses props.eventId, another uses props.gameId, another uses both of these, another uses props.eventId and props.sportId; all selectors will just work correctly as long as I make sure the props object contains all of the values it needs.

So I'm hoping to get clarification on whether this approach works the way I think it does and has the potential performance losses that would come as a result, and whether this is something I should worry about or whether the performance losses are negligible and outweighed by the improved productivity/flexibility during development when using the props object as the only argument to my selectors like this. Ideally, I think the docs should cover the possible approaches for selectors that take arguments (the approach of passing a single props object which I've described here, or the approach of passing multiple specific props directly), and the pros/cons and considerations for these approaches.

[Amurmurmur]

Whats the best practice?
Is it best to use selectors in which computations are done ?
Or have a computation done with data on an api response say in a saga and store modified data in a store ?
If for instance I need to do any mapping, reducing of the data I've received from the API.

[joedski]

As per the description of createSelector, (which honestly might take a few or 10 readings to fully understand) while a Selector does check the inputs passed to it, it also checks the results of each Input-Selector. This means a prop-extractor selector like (state, props) => props.eventId will not result in a recalculation so long as eventId does not change.

This can also be deduced from the use with Redux: If a selector recalculated the output on every input, and not based on the values of the Input-Selectors, then every State Change would cause every Selector in the app to have to recalculate.

[joedski]

@Amurmurmur My own opinion is: Do as little as possible, and do it as late as possible.

If you want denormalized data, then don't normalize it. (And if that means you end up not needing Redux, then leave it out.) If you want normalized data, don't later denormalize it, just work directly with the normalized data and connect() as necessary. The less you transform data, the fewer redraws you risk.

For lists of things, use cached arrays of ids (cached at the component instance, preferably) and delegate getting of the actual items to the children, where they can then decide if they need to update.

[jordanmkoncz]

@joedski thanks for the response, that does make sense.

Interestingly, the description of the createSelector API now actually shows an example of passing a component's props to your selectors exactly like I've been doing, under the "It can be useful to access the props of a component from within a selector..." section. I think this part of the docs was added/updated some time after I originally created this issue, or I somehow missed this section of the docs, but either way it's good to have an example of this approach shown in the docs.

Are you aware of any downsides or possible pitfalls with this approach? It seems like this approach of always just passing a props object as the second argument to your selectors has massive advantages over the approach of passing specific props as separate arguments to your selectors, i.e.:

// Approach 1: pass the props object.
EventListItem = connect((state, props) => ({
  eventName: selectors.selectedEventName(state, props),
}))(EventListItem);

// Approach 2: pass specific props.
// With this approach, the `selectedEventName` selector would obviously need to be updated to
// use the `eventId` and `gameId` values that are passed as separate arguments.
EventListItem = connect((state, props) => ({
  eventName: selectors.selectedEventName(state, props.eventId, props.gameId),
}))(EventListItem);

Are there any benefits to Approach 2, or any situations where it would be the better choice?

[markerikson]

The issue with approach 1 is that the props object is going to be a different reference 99% of the time, so your selector won't memoize properly. So, you've go two realistic options to fix that:

• Manually pass in multiple props, per your Approach 2 example
• Write one or more "input selectors" that extract the needed values from the props object.

[joedski]

As @markerikson said, approach 1 will work properly so long as one of the input-selectors to that selectedEventName selector extracts the appropriate prop off of the component's props.

I personally would use approach 2 for globally defined selectors, though, since approach 1 requires that any component using that selector has the expected arguments on the expected prop names. Which is probably a fair expectation, but I personally don't like that semi-hidden contract; I prefer the explicit parametrization of approach 2. It also gets around if for some reason a component must deal with two ids for the same entity type, for example props.prevEventId and props.nextEventId, etc.

Where I would use approach 1 is when defining component-specific selectors, where one (or more) of their input selectors is a selector that extracts a prop from the props object. In that case, what prop is on what prop-name is still specific to the component, but with those selectors being also specific to the component rather than global you don't place pre-conditions on other components about how their props must be named.

[jordanmkoncz]

@markerikson @joedski thanks for the clarification.

The situation you're describing where the props passed to some component are not already in the form that the selectors expect, e.g. props.nextEventId rather than props.eventId, is one that I've run into before. In those cases, in my mapStateToProps function I've just constructed a new props object that is in the form I need, like so:

EventListItem = connect((state, props) => {
  const eventProps = {
    eventId: props.nextEventId,
  };

  return {
    eventName: selectors.selectedEventName(state, eventProps),
  }
}))(EventListItem);

I guess this is the alternative way to handle that situation of a component receiving props with different prop names while keeping all your selectors consistent with Approach 1.

[markerikson]

Yeah, the issue with that can be that you _are_ constructing a new reference and passing it to the selector, so depending on what the input selectors do with that object, you may be keeping it from memoizing properly.

[Soundvessel]

I have what might be a related question.

If I have a prop selector in my module as:

const selectSkillId = (state, props) => props.skillId

Is there any advantage to just passing what I need in the mapState like so?

const mapState(state, props) {
  return {
    // does it save memory etc by not passing the all props? The object here retains multi-prop composibility.
    selectSkill(state, { skillId }),
  }
}

You could even go so far to destructure at the mapState function arg level. Either way, I don't see object reference being an issue as the Id selector will extract the same value which is checked in a later reselect or re-reselect cached selector?

const mapState(state, { skillId }) {
  return {
    selectSkill(state, { skillId })
  }
}

I understand using all props is easier in terms of selector compostability. So the only downside I see is when additional props are necessary, you must, in addition to applying them to the component, add them to its mapState. Perhaps a reasonable tradeoff?

One more idea while I am here. We use the ducks pattern giving each entity type in Redux their own module. For nested entities, I found using a selectEntities with denormalize() in a selector busted the cache on every entity change. It also seemed to break the rule of ducks resulting in one entity dealing with properties of another in the parent's entities' selector.

Would compose be an acceptable way to immediately take returned ids from the first entity and pass into a second connect function? In this example, the first selector would return skillId so the second connect function can do its lookup properly. This would be for instances where separately connected sub-component isn't necessary. This seems to better focus denormalize where we need it for each entity type.

```js
import { compose } from 'redux'

const SomeComponent = compose(
connect(mapState), // selectSomething, returns skillId with something props
connect(mapSubState), // selectSkill, uses skillId prop from first connect
)(SomeConnectedComponent)

[joedski]

I'm a little late to reply here...

I have what might be a related question.

If I have a prop selector in my module as:

const selectSkillId = (state, props) => props.skillId

Is there any advantage to just passing what I need in the mapState like so?

const mapState(state, props) {
  return {
    // does it save memory etc by not passing the all props? The object here retains multi-prop composibility.
    selectSkill(state, { skillId }),
  }
}

You could even go so far to destructure at the mapState function arg level. Either way, I don't see object reference being an issue as the Id selector will extract the same value which is checked in a later reselect or re-reselect cached selector?

const mapState(state, { skillId }) {
  return {
    selectSkill(state, { skillId })
  }
}

Where are you trying to save what memory? Could you expand on that?

Otherwise, if you wanted to save memory, just don't create a new object. Selectors that pick props will be happy with any object that has the desired prop name, whether that's a new object in memory or the original props object. Naturally, if you have to rename the prop (e.g. skillId vs prevSkillId) then you'll have to create a new object anyway...

And of course, if you're dealing with poor memory and computation performance, be sure to profile things first to see what's actually eating resources.

I think I'll need a full example to see what you're really talking about, though. Sometimes things are not entirely evident from description plus excerpts.

I understand using all props is easier in terms of selector compostability. So the only downside I see is when additional props are necessary, you must, in addition to applying them to the component, add them to its mapState. Perhaps a reasonable tradeoff?

Using all props is easier if all selectors target the same prop name, otherwise you need to rename the props, which means creating a new object.

I'd say in general, focus first on making it work, then bring efficiency concerns to it later when you've had a moment to step back and look for emerging patterns. Sometimes that means changing things after the fact to fit them into given patterns.

One more idea while I am here. We use the ducks pattern giving each entity type in Redux their own module. For nested entities, I found using a selectEntities with denormalize() in a selector busted the cache on every entity change. It also seemed to break the rule of ducks resulting in one entity dealing with properties of another in the parent's entities' selector.

Would compose be an acceptable way to immediately take returned ids from the first entity and pass into a second connect function? In this example, the first selector would return skillId so the second connect function can do its lookup properly. This would be for instances where separately connected sub-component isn't necessary. This seems to better focus denormalize where we need it for each entity type.

import { compose } from 'redux'

const SomeComponent = compose(
  connect(mapState), // selectSomething, returns skillId with something props
  connect(mapSubState), // selectSkill, uses skillId prop from first connect
)(SomeConnectedComponent)

If it works, then it's acceptable. Probably. Each layer of connect() adds another React component, though, so do keep that in mind.

I'm not exactly sure what each of those is doing from your description here, though. Is mapState here something like (state, ownProps) => ({ skillId: ownProps.skillId })? I'm not sure what good that would do since that would be entirely redundant with when connect determins it should try to redraw the wrapped component based on ownProps. The inner connect(mapSubState) will see ownProps.skillId with or without the connect((state, ownProps) => ({ skillId: ownProps.skillId })).

Perhaps I'm missing something here, and a more fully expanded example would better explain things.

Regarding denormalize generally, I'll repeat what I said above: in general, do as little as possible and do it as late as possible. If you already normalize all your data, then try to avoid denormalizing it and just work with the source data instead. If you absolutely must denormalize it, denormalize it as close as possible to the point that needs the denormalized entity, and no further from it. As you noted, denormalization can cause undesired updates, because the computation of a denormalized entity is dependent on every source datum used to compute it.

In turn, if a denormalized entity must be recomputed every time its source data change, then any of the source data changing should cause a redraw anyway, which means it is the same result whether you're invalidating the cache based on the source data or the denormalized result.

I might even go so far as to say, if you're dealing with normalized data, you're better off treating denormalization as a part of the render step rather than as part of the state tracking step. Computing the denormalized entity likely a lot cheaper than any sort of processing the underlying rendering library has to do to flush changes to the screen, anyway.