Reselect: Is it a bad pattern to use the props inside of the selectors?

[loick]

Hi everyone,

I'm using Reselect for a while now, and the more my codebase is growing, the more I'm wondering if I'm not doing something wrong with the props.

Let me explain, on the my redux store, I keep some entities stored by ids, and on a url like /entity/:id, I would like to retrieve the entity object.

Example:

const getEntityIdFromUrl = (_, ownProps) => ownProps.match.params.entityId // Using react router
const getEntities = state => state.entities

export const getCurrentEntity = createSelector(
  getEntities,
  getEntityIdFromUrl,
  (entities, id) => entities[id]
)

It is working very well... If I'm on the right context!

The selector is bound to a store which is a global object for my application. Introducing the props, and especially here with the routing props, bounds it to a context.

getCurrentEntity // will break if I don't have the param "entityId"

So I'm wondering, is it a bad pattern to use the props like that into the selectors? Or should I specify to my team that some selectors should only be used in the right context?

[markerikson]

It's kind of up to you. I personally would prefer to have the component's mapState be the one that defines what piece of the props is needed, ie:

const mapState = (state, ownProps) => {
    return {
        currentEntity : getCurrentEntity(state, ownProps.match.params.entityId)
    }
}

I see it kind of like dealing with event handlers. You probably don't want to pass the entire event object all the way through to whatever your "business logic" is. The UI ought to deal with extracting the relevant data from the event, like const name = e.target.value, and then pass that onward to the rest of the logic.

[loick]

This is actually the exact implementation I had before moving the props to the selector file, in order to have getCurrentEntity as a variable, instead of a function (supposed to be more efficient right?).

That's why I ended up a bit confused on the best approach to have here.

Moreover, the mapState function could be tricky to use like that all the time, since it is not updated when the props are changing (so this is definitely something to be careful of).

Anyway, what do you think, functions over props in the selectors are better in your opinion?

[Flufd]

I found that I didn't like passing all of the props into the selectors as the second parameter either, because I felt that I was binding my selectors to the implementation of react-router and any other parent component that was passing props in. I wanted to control what was being passed to the selectors so that they could be reused elsewhere in the app (components not wrapped in a Route component etc), and my selector testing code wouldn't have to create mocks of the whole react-router match prop tree. So I initially just passed parameters in as positional arguments.

But when I needed to pass multiple parameters it I felt that it became unclear what I was selecting using selectors like...

const getSecondParameter = (state, param) => param
const getThirdParameter = (state, _, param) => param

...and I was relying on the order of the parameters in multiple nested selectors.

Let's say I have

• selector A, it takes the state and a customer Id as arguments.
• selector B, it takes the result of selector A, and the order status filter as arguments

If I pass in each parameter to the call to the selector individually, selector B must access the order filter argument via the third parameter using getThirdParameter above, or more clearly:

const getOrderStatusParameter = (state, _, orderStatus) => orderStatus

That's not so bad, but this starts to get really rigid once there are a few different nested selectors expecting parameters, if I want to reuse a sub-selector in another place, it needs to make sure to have the argument in the correct position.

Example:
Lets say I have another selector

• selector C that was used elsewhere, it takes the state and a product Id as the second param

Now I want to re-use selector C in a new selector D, which takes selector B and C as inputs. However the parameters for A (used by B) and C are in clashing positions, but I've already used C in multiple places within the app...

My first thought was, "we can pass all of the parameters in at once"...

const selectorD = createSelector(
    state => state, 
    (_, params) =>  params,
   (state, { Id, filter, somethingElse }) => {
        // Do expensive filtering
    }
)

const mapStateToProps = (state, { x, y, z }) => {
    return {        
        data: selectorD(state, { x, y, z })           
        })        
    }
}

...but now the memoization breaks because the mapStateToProps creates a new javascript object each time.

Eventually I went with a mixture of the two approaches above, always passing in a shallow javascript object as the second parameter and having a selector extract each parameter that I needed from that. This also made the final selectors clearer because the parameter names are just before the output function of createSelector

// Example: I want to show the pending orders for a customer, 
// but this could be something like passing multiple filter values to filter a large list
// something we want memoized

// url is something like /customer/1/orders/pending
// path match pattern is /customer/:customerId/orders/:status
const mapStateToProps = (state, { match }) => {
    return {        
        orders: getCustomerOrdersOfStatus(state, { 
            customerId: match.params.customerId, 
            orderStatus: match.params.status
        })        
    }
}

// I have selectors for each slice of the app state
const getCustomers = (state) => state.customers
const getOrders = (state) => state.orders

// I have some selectors like this to select the parameters I need
// these are reused throughout the app
const getCustomerIdParam = (_, params) => params.customerId
const getOrderStatusParam = (_, params) => params.orderStatus

// And use them in the main selector
const getCustomerOrdersOfStatus = createSelector(
    // Select the slices of state
    getCustomers,    
    getOrders,
    // Select the params
    getCustomerIdParam,
    getOrderStatusParam
    (customers, orders, customerId, orderStatus) => {
        // Some expensive filtering here
})

Note that, because the expensive output selector never receives the newly created javascript object passed into the second argument of the selector, the default memoization implementation still works, because the parameters passed to the final selector are simple primitives.

Does this solution have any drawbacks that I've missed?