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

Selector with dynamic argument not memoized #392

Closed
watch-janick opened this issue Jan 13, 2019 · 5 comments
Closed

Selector with dynamic argument not memoized #392

watch-janick opened this issue Jan 13, 2019 · 5 comments

Comments

@watch-janick
Copy link

watch-janick commented Jan 13, 2019

Hello,

Sorry if this has already been discussed but I can't find a way to make it work for my use case.

My selector:

import { createSelector, defaultMemoize } from 'reselect'

const allStatesSelector = store => store.state.states

export const getAllStates = createSelector(
  allStatesSelector,
  states => defaultMemoize(night => {
    const availableStates = night ? nightStates : dayStates
    return states.filter(state => availableStates.includes(state.id))
  }),
)

I'm calling it that way:

getAllStates(store)(props.night)

The selector is executed 1800 times, instead of 2 times (because there's only 2 possibilities, night is either true or false).

What did I miss?

@markerikson
Copy link
Contributor

Two questions:

  • Why are you calling defaultMemoize() yourself?
  • Why are you having your "output selector" return a separate function?

Also:

  • What does your data look like?
  • Where are you counting "1800 times"?
  • Where and when are you calling the selector?

@watch-janick
Copy link
Author

watch-janick commented Jan 13, 2019

Hello!

I've tried many things hence why I ended up with this code xD

This version seemed more legit but still was being excecuted ~1800 times (which is the number of call I'm making to this selector during one render).

import { createSelector } from 'reselect'

const allStatesSelector = (store, night) => {
  const { states } = store.state

  console.log('allStatesSelector executed with night = ', night)

  const availableStates = night ? nightStates : dayStates
  return states.filter(state => availableStates.includes(state.id))
}

export const getAllStates = createSelector(
  allStatesSelector,
  states => states,
)

Called like so:

getAllStates(state, props.night)

My data looks like :

nightStates:

const nightStates = [5, 6, 7, 10, 11, 12, 13]

dayStates:

const nightStates = [1, 2, 3, 4, 6, 7, 10, 11, 12, 13]

store.state.states:

{
  states: [
    {
      isWorking: true,
      isRegularWorking: true,
      id: 1,
      display: '0',
      hours: 8
    },
    {
      isWorking: true,
      isRegularWorking: true,
      id: 2,
      display: '0',
      hours: 8
    },
    {
      
      isWorking: true,
      isRegularWorking: true,
      id: 3,
      display: 'C',
      hours: 10
    },
    {
      
      isWorking: true,
      isRegularWorking: true,
      id: 4,
      display: 'C',
      hours: 10
    },
    {
      
      isWorking: true,
      isRegularWorking: true,
      id: 5,
      display: 'C',
      hours: 12
    },
    {
      
      isWorking: true,
      isRegularWorking: false,
      id: 6,
      display: 'C',
      hours: 12
    },
    {
      
      isWorking: false,
      isRegularWorking: false,
      id: 7,
      display: 'A',
      hours: 8
    },
    {
      
      isWorking: false,
      isRegularWorking: false,
      id: 10,
      display: 'R',
      hours: 0
    },
    {
      
      isWorking: false,
      isRegularWorking: false,
      id: 11,
      display: 'V',
      hours: 0
    },
    {
      
      isWorking: false,
      isRegularWorking: false,
      id: 12,
      display: 'S',
      hours: 0
    },
    {
      
      isWorking: false,
      isRegularWorking: false,
      id: 13,
      display: 'M',
      hours: 0
    }
  ]
}

The selector is called in a component displayed 1800 times in the page (basically that's a table cell).
I know that I could retrieve the selector on the parent component and pass it to the cell, but it'll still be called multiple times and I want to get this memoization to work anyway :D

@markerikson
Copy link
Contributor

Okay.

First, it's important to understand that Reselect takes the "input selectors" you provide, and calls them every time. It then inspects the results of those "input selectors". If any of the return values are different than last time, then it calls the "output selector" with all of those values as arguments. If all of the return values are the same as before, then it skips calling the output selector and returns the cached result.

So, your allStatesSelector is being used as an "input selector", and thus will be called every time.

The way to think about this is, "I only want my output selector to run whenever one of these values changes".

Similarly, you don't ever want to have an "output selector" that just returns the argument, like state => state. That's not doing anything useful.

Also, you can pass multiple arguments to a Reselect selector, but you then need to write "input selectors" that extract those arguments and pass them on to the "output selector".

So, as a basic example:

const selectAllStates = state => state.states;
const selectIsNight = (state, isNight) => isNight;

const selectMatchingStates = createSelector(
    [selectAllStates, selectIsNight],
    (allStates, isNight) => {
        const availableStates = isNight? nightStates : dayStates
        return states.filter(state => availableStates.includes(state.id))
    }
);

You might want to read my post Idiomatic Redux: Using Reselect Selectors for Encapsulation and Performance for some further guidance.

@watch-janick
Copy link
Author

Great thank you!

It's still not working but I know why now... It's because my selectors are called in an alternance of night = true and night = false.

I thought the memoization would take place for every argument, like if you called it with a certain set of arguments, it will memoize it until the store change and not only if it was called with the same arguments last time :/

@markerikson
Copy link
Contributor

Yup. If you need to call the same selector multiple times in a row with different arguments, you'll need to create distinct instances of that selector so they each memoize properly. See the "factory functions" section of the post I linked for details on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants