API design debate: selectors field in createSlice

[markerikson]

In #3382 , we added a selectors field inside of createSlice, with an associated slice.getSelectors(selectState) method. Currently, that field is pretty limited. Selectors inside of it assume they get the slice state as an arg, but right now we're not sure about the right way to allow memoizing selectors in that field.

We had an internal discussion about potential selectors field API design and use cases earlier today, which I am pasting here for reference:

phryneas : The only question that is still open might be this one:

How do we allow for memoized selectors?

phryneas : I was thinking... @acemarke, could we add a .duplicate function on reselect selectors, so a second call to .getSelectors() could get a different set of memoized selectors?
acemarke : oh. that's a good point. those aren't memoized are they
eskimojo : do they need to be?
acemarke : very potentially
eskimojo : you can pass the result of a createSelector call to selector or inside a selector object
phryneas : Could be nice if someone put reselect selectors into those slice selecors
acemarke : if you're doing anything more than returning a value
phryneas : Exactly my thought, but then you'd want to have individual instances for different .getSelectors calls.
eskimojo : hmm
phryneas : (Although it is debatable if it makes any sense to have different .getSelector calls)
eskimojo : yeah i was gonna say
eskimojo : chances are you're only gonna have one set
phryneas : But just in case, a .duplicate api on reselect selectors could take care of that
phryneas : mostly, yes - although there might be the case where someone would have different selectors for the real path and with an identity selector as input, to do things
eskimojo : i'm also curious if proxy-memoize might run into issues with our state proxy - i had issues with using it and immer together
phryneas : which might maybe sense with draftSafeSelectors :think:
eskimojo : i love cans of worms
phryneas : ^^
eskimojo :

const slice = createSlice({
  name,
  initialState,
  selectors: {
    selectSlice: (state) => state,
  },
  selectorFactories: {
    selectMemoized: (arg) => createSelector([(slice) => slice], (slice) => slice.length)
  }
})

const { selectSlice } = slice.selectors;

const { createSelectMemoized } = slice.selectorFactories

const selectMemoizedInstance = createSelectMemoized(arg);

phryneas : That would create a new memoized selector on each run
acemarke : :/
eskimojo : why?
phryneas : Oh sorry, missed the selectorFactory
phryneas : I'd rather think something along the lines of

  selectors: {
    selectMappedItems: createDraftSafeSelector(slice => slice.items, items => items.map(item => item.title)
  },

const draftSelectors = slice.getSelectors(slice => slice)
const globalSelectors = slice.getSelectors(state => state.slice)

eskimojo : sure, i'm just trying to think of something that doesn't depend on the memoize library having a method of duplicating the selector from the selector itself
acemarke : feels like selectors should be a callback that gets selectSlice as an argument?
phryneas : Might also be an option, but also complicated things.
acemarke : (I'm still not terribly keen on the whole "draft-safe selector" thing in general tbh)
phryneas : I'd rather say we have some kind of option that if a selector has a duplicate property, that will be called on getSelectors to create a copy
acemarke : I don't like that idea :(
eskimojo : which enforces a dependency on reselect
phryneas : I know, but people want to use selectors in reducers
eskimojo : what if i wanted to use proxy-memoize?
phryneas : or any other library that would have some duplicate property. Could be added, could be wrapped somehow... dunno
eskimojo : are there any existing selector libraries that have a duplicate property?
acemarke : nope
phryneas : no, we would create some new pattern there
phryneas : although it might be generally a useful pattern which would allow components to get their per-component-memoized instance of an existing selectory
phryneas :
const foo = useSelector(useMemo(someSelector.duplicate, []))

acemarke : I'm sorta hoping that hand-wave-y WeakMap stuff might let us bypass that cache size issue in Reselect v5, but it's still hazy
acemarke : a couple other things I'm not sure we're accounting for:

• selecting from other parts of the root state
• passing in arguments

phryneas : passing in arguments would not be a problem, but there is no selecting other parts of the root state. you would rather use these as building blocks for more complicated selectors
eskimojo : the current slice selectors allow for additional arguments, but they will only ever receive the slice state as their first argument
eskimojo : anything that needs more should just be defined outside the slice
eskimojo : (not least because allowing RootState to be used in slice selectors runs into circular type issues)
eskimojo : .getSelectors allows us to move the RootState part to a separate place that doesn't get used when determining RootState, thus no circle

So, the open questions are:

• Need for memoization
• Per-component-instance selectors / factories
• and to a lesser extent, hypothetically being able to select from the root state

We'd appreciate feedback on potential API design and use cases here!

[agusterodin]

Just giving context on how i'm currently doing reusable slices and accompanying selectors. The current way I have them implemented (pre-alpha 5) feels extreme and messy.

Defining a reusable set of selectors

export function createGeoselectionMapSelectors<State>(stateAccessor: (rootState: State) => GeoselectionMapState) {
  const basicSelectors = {
    getViewport: (state: State) => stateAccessor(state).viewport,
    getBounds: (state: State) => stateAccessor(state).bounds,
    getMaxBoundsZoom: (state: State) => stateAccessor(state).maxBoundsZoom,
    getStyle: (state: State) => stateAccessor(state).style,
    getSelections: (state: State) => stateAccessor(state).selections,
    getSelectionCount: (state: State) => stateAccessor(state).selections.length,
    getHasSelections: (state: State) => stateAccessor(state).selections.length > 0,
    getDrawMode: (state: State) => stateAccessor(state).drawMode,
    getIsEditing: (state: State) => stateAccessor(state).drawMode === LeafletDrawMode.Edit,
    getIsDrawing: (state: State) =>
      stateAccessor(state).drawMode === LeafletDrawMode.Polygon || stateAccessor(state).drawMode === LeafletDrawMode.Circle,
    getSelectionTool: (state: State) => stateAccessor(state).selectionTool,
    getAnimated: (state: State) => stateAccessor(state).animated,
    getPlaceSearchText: (state: State) => stateAccessor(state).placeSearchText,
    getPlaceSearchResults: (state: State) => stateAccessor(state).placeSearchResults,
    getUnpinnedPlaceMarkers: (state: State) => stateAccessor(state).unpinnedPlaceMarkers,
    getImportedPlaceMarkers: (state: State) => stateAccessor(state).importedPlaceMarkers,
    getUnpinnedPlacePopup: (state: State) => stateAccessor(state).unpinnedPlacePopup,
    getImportedPlacePopup: (state: State) => stateAccessor(state).importedPlacePopup,
    getEditSelectionIndex: (state: State) => stateAccessor(state).editSelectionIndex,
    getAddMarkerDialogVisible: (state: State) => stateAccessor(state).addMarkerDialogVisible,
    getAddCircleDialogVisible: (state: State) => stateAccessor(state).addCircleDialogVisible,
    getDialogVisible: (state: State) => stateAccessor(state).addMarkerDialogVisible || stateAccessor(state).addCircleDialogVisible,
    getMousePosition: (state: State) => stateAccessor(state).mousePosition
  }

  const getSelectionsBoundingBox = createSelector([basicSelectors.getSelections], selections => {
    return getSelectionsBounds(selections)
  })

  const getAllSelectionsHaveCustomTimeframes = createSelector([basicSelectors.getSelections], selections => {
    const selectionWithoutCustomTimeframePresentFound = selections.some(selection => !selection.startDate || !selection.endDate)
    return !selectionWithoutCustomTimeframePresentFound
  })

  return {
    ...basicSelectors,
    getSelectionsBoundingBox,
    getAllSelectionsHaveCustomTimeframes
  }
}

Setting up the slice into application state

import { LeafletDrawMode } from 'common/components/map'
import { createGeoselectionMapSelectors, createGeoselectionMapSlice } from 'common/components/map/geoselection/geoselectionMap.slice'
import { RootState } from 'state/store'

export const campaignCreatorMapSelectors = createGeoselectionMapSelectors<RootState>(state => state.creator.targeting.geofencing)

const geoselectionMapSlice = createGeoselectionMapSlice({
  name: 'creator/targeting/geofencing',
  initialStateCustomizations: { selectionTool: LeafletDrawMode.Circle }
})

export const campaignCreatorMapActions = geoselectionMapSlice.actions

export default geoselectionMapSlice.reducer

Example of reusable component in our monorepo "common" library that takes the selectors and actions

import { useDispatch, useSelector } from 'react-redux'
import classNames from 'classnames'
import { GeoselectionMapActions, GeoselectionMapSelectors } from './geoselectionMap.slice'

interface Props {
  geoselectionMapSelectors: GeoselectionMapSelectors
  geoselectionMapActions: GeoselectionMapActions
}

export default function GeoselectionsClearAllButton({  geoselectionMapSelectors, geoselectionMapActions }: Props) {
  const dispatch = useDispatch()
  const hasSelections = useSelector(geoselectionMapSelectors.getHasSelections)

  if (hasSelections) {
    return (
      <button
        className={classNames(
          'carbon-button gray mx-auto mt-4 mb-3 px-7 py-[11px] !text-gray-600 hover:!text-gray-700 hover:!bg-gray-300 font-medium rounded-full opacity-100 duration-[90ms] transition-opacity',
        )}
        onClick={() => dispatch(geoselectionMapActions.deleteAllSelections())}
      >
        Clear all
      </button>
    )
  }

  return null
}

This is how we are currently doing reusable slices/selectors

So if you added a "getSelectors" function to the output of createSlice function, I would

1. Add the "simple" selectors directly into createSlice selectors field
2. Change my "create selectors function" to the below code?

export function createGeoselectionMapSliceSelectors() {
  const basicSelectors = geoselectionSlice.getSelectors()

  const getSelectionsBoundingBox = createSelector([basicSelectors.getSelections], selections => {
    return getSelectionsBounds(selections)
  })

  const getAllSelectionsHaveCustomTimeframes = createSelector([basicSelectors.getSelections], selections => {
    const selectionWithoutCustomTimeframePresentFound = selections.some(selection => !selection.startDate || !selection.endDate)
    return !selectionWithoutCustomTimeframePresentFound
  })

  return {
    ...basicSelectors,
    getSelectionsBoundingBox,
    getAllSelectionsHaveCustomTimeframes
  }
}

I see that API design is still up in the air. That being said, would my above assumption be correct assuming you decided to go the getSelectors() route?

[EskiMojo14]

the current implementation has getSelectors - slice.selectors is essentially an alias for slice.getSelectors(rootState => rootState[slice.reducerPath])(slice.reducerPath defaults to slice.name if unspecified in the slice config)

getSelectors without any selectState parameter passed returns each selector essentially as it is - expecting the slice state as its first parameter

const counterSlice = createSlice({
  name: 'counter',
  initialState: { value: 0 },
  reducers: {},
  selectors: {
    selectValue: (sliceState) => sliceState.value
  }
})

counterSlice.selectors.selectValue({ counter: { value: 0 } })
counterSlice.getSelectors().selectValue({ value: 0 })
counterSlice.getSelectors((state) => state.aCounter).selectValue({ aCounter: { value: 0 } })

but other than that, your assumption seems correct to me - all your selectors could be moved in the slice and getSelector would take the stateAccessor parameter

[markerikson]

I'll toss in a general note per https://redux.js.org/usage/deriving-data-selectors#balance-selector-usage :

Adding a separate selector function for every single field is not a good idea! That ends up turning Redux into something resembling a Java class with getter/setter functions for every field. It's not going to improve the code, and it's probably going to make the code worse - maintaining all those extra selectors is a lot of additional effort, and it will be harder to trace what values are being used where.

I realize there's definitely times when you do want just an individual slice state field, and don't want to grab the entire slice state because it'll re-render too often.

That's part of why I keep debating if it's worth having some kind of autotracking/proxy/signal-based API built into either RTK or React-Redux, because there are certainly times when it would be simpler from a usage perspective to write <div>{state.mySlice.someField}</div> and not have to write a separate selector.

[phryneas]

I realize there's definitely times when you do want just an individual slice state field, and don't want to grab the entire slice state because it'll re-render too often.

There is also the new use case "you want a single field and it is an injected slice, so now you need a selector".

[markerikson]

We're getting close to having RTK 2.0 done (per the 2.0 milestone issues list). The major remaining items have to do with nailing down final API design on the new features we've added, like the callback syntax for createSlice.reducers and how it uses createAsyncThunk.

Specifically here, we have open questions around the new createSlice.selectors field. In its current form, it doesn't let you create unique selector instances. Accessing someSlice.selectors creates a set of selector functions (with the default state => state[slice.reducerPath] lookup) and caches them. Calling someSlice.getSelectors(selectState?) will cache the results based on the selectState reference.

There are cases when you want to create multiple unique instances of a selector. This is most commonly because Reselect selectors have a default cache size of 1, and using the same selector instance in different components with different arguments passed in will cause the memoization to repeatedly fail and recalculate a different result every time.

Reselect does now have a maxSize option, and there's also a new weakmapMemoize memoizer that has a larger (theoretically infinite?) cache size. But, we want to try to allow for people to create unique selector instances if needed.

Right now we've got two different PRs with different possible approaches (example selectors are kinda pointless and here just to illustrate the difference in syntax):

• Add selector factories to createSlice #3671 : adds createSlice.selectorFactories. This is a field that takes selectThing: () => Selector, and turns that into someSlice.selectorFactories.makeSelectThing: () => Selector

const sliceWithSelectors = createSlice({
  name: 'counter',
  initialState: { value: 0 },
  reducers: {
    increment: (state) => {
      state.value += 1
    },
  },
  selectors: {
    selectValue: (state) => state.value,
  },
  selectorFactories: {
    selectMultiply: () =>
      createSelector(
        (state: { value: number }) => state.value,
        (_: unknown, multiplier: number) => multiplier,
        (value, multiplier) => value * multiplier
      ),
  },
})

export const { makeSelectMultiply } = sliceWithSelectors.selectorFactories;
export const oneSelectMultiplyInstance = makeSelectMultiply();

• Explore simpler selector factory idea #3836: alters createSlice.selectors to accept selectThing: Selector | {factory: () => Selector}, and also changes the internal logic to generate a new set of selectors every time someSlice.getSelectors() is called

const sliceWithSelectors = createSlice({
  name: 'counter',
  initialState: { value: 0 },
  reducers: {
    increment: (state) => {
      state.value += 1
    },
  },
  selectors: {
    selectValue: (state) => state.value,
    selectMultiply: {
      factory: () =>
        createSelector(
          (state: { value: number }) => state.value,
          (_: unknown, multiplier: number) => multiplier,
          (value, multiplier) => value * multiplier
        )
    }
  },
})

// call .getSelectors() each time you need a new instance
export const oneSelectMultiplyInstance = sliceWithSelectors.getSelectors( (state) => state.counter )

Tradeoffs

For definitions, I think #3836 is probably simpler to read and use. One createSlice.selectors field, with some flexibility about how you define them.

On the other hand, with #3671 the usage to make new selectors is probably simpler:

import { makeSelectItemsByCategory } from './categoriesSlice'

function CategoryList({ category }) {
  // Create a new memoized selector, for each component instance, on mount
  const selectItemsByCategory = useMemo(makeSelectItemsByCategory, [])

  const itemsByCategory = useSelector(state =>
    selectItemsByCategory(state, category)
  )
}

whereas with #3836 it's more like:

import { categorySlice } from './categoriesSlice'

function CategoryList({ category }) {
  // Create a new memoized selector, for each component instance, on mount
  const selectItemsByCategory = useMemo(() => categorySlice.getSelectors(selectSlice).selectItemsByCategory, [])

  const itemsByCategory = useSelector(state =>
    selectItemsByCategory(state, category)
  )
}

So, outstanding questions:

• What use cases exist for creating multiple instances of a selector?
• What syntax do you prefer as far as defining a selector factory?
• What syntax do you prefer as far as using a selector factory?
• Any other API design constraints or suggestions we should be considering here?

[EskiMojo14]

For definitions, I think #3836 is probably simpler to read and use. One createSlice.selectors field, with some flexibility about how you define them.

On the other hand, with #3671 the usage to make new selectors is probably simpler

fwiw this is why i prefer selectorFactories as a dedicated field. you'll define and export the factory once, but you'll likely want the selector multiple places. in my eyes that makes the usage DX more important than the definition DX

[aryaemami59]

Hey I have some questions regarding createSlice.selectors. I understand what you guys are trying to do for the most part. The syntax pattern resemples the way entity adapter selectors work which is good because it's simple and it works. But is there a reason the slice selectors can't accept a custom memoize function? and I read the section where you talked about creating unique selector instances, but I'm just trying to understand why that is necessary. I think you under value the weakMapMemoize method that you implmeneted. It is insanely good. It's very fast and drastically cuts down on recalculations. I think the best thing to do in order to provide a simple yet effective selector api is to expose the
custom memoization options (CreateSelectorOptions), first at the slice level, (kind of the same way the new function signature
for createSelectorCreator works), so that those options get applied to all the selectors in the slice. Then the second parameter for slice.getSelectors, can also be of type CreateSelectorOptions to allow us to override those options per selector. That would be my take on it, and if you want, I can help with implementation as well. As always let me know if there is anything I can help with.

[EskiMojo14]

generally people will use proxy-memoize instead of reselect, not as well as.

We use createSelector for entity adapters because some of the selectors (selectAll, and kinda selectTotal) it creates explicitly do need memoization. some of the selectors created in entity adapters could be switched to not use createSelector, and there would be no meaningful change in behaviour (and a slight performance improvement) - they just use it for easier composition.

the question of memoization in slice selectors is not whether we need it, it's how we best support input selectors that are themselves memoized.

[aryaemami59]

generally people will use proxy-memoize instead of reselect, not as well as.

Sorry could you clarify this? are you saying more people use proxy-memoize instead of reselect?

the question of memoization in slice selectors is not whether we need it, it's how we best support input selectors that are themselves memoized.

If that is the case, creating a unique selector instance in the selector factory examples that is shown will probably break memoization. That's why I suggested the use of weakMapMemoize. I still think when given the choice, more people would opt in for memoization than the other way around. The only difference between

const selectAllCarts = ({ cart }) => cart.ids.map(id => cart.entities[id]!)

And this

const slice = createSlice({
  name: "cart",
  initialState,
  selectors: {
    selectAllCarts: state => state.ids.map(id => state.entities[id]!),
  },
  reducers: {},
})

is the location in which it is declared. I still think it would probably be good to at least allow people to pass in a custom selectorCreator to slice.getSelectors.

[markerikson]

@aryaemami59 to clarify:

• Reselect is the default standard selector library used in Redux apps (my guesstimate would be 98% Reselect usage, 2% proxy-memoize usage tops)
• But we do want to leave the option of customzing how a selector gets instantiated here in createSlice, and not force it to be Reselect or the default createSelector behavior
• which is really the entire point of this discussion in the first place :) in other words, we are debating what the right syntax ought to be for that use case

[EskiMojo14]

Sorry could you clarify this? are you saying more people use proxy-memoize instead of reselect?

no, i'm saying that the people who choose to use proxy-memoize are doing so as an alternative to reselect, so it would be odd to use one in combination with the other.

[aryaemami59]

• Reselect is the default standard selector library used in Redux apps (my guesstimate would be 98% Reselect usage, 2% proxy-memoize usage tops)

@markerikson Thanks for the clarification. Sorry for the confusion.

• But we do want to leave the option of customzing how a selector gets instantiated here in createSlice, and not force it to be Reselect or the default createSelector behavior
• which is really the entire point of this discussion in the first place :) in other words, we are debating what the right syntax ought to be for that use case

Understandable. You could make it so that instead of having to manually define a selector factory inside createSlice, they just get generated for each selector so that way you don't have to define a selector and a selector factory everytime you wanted a new instance, but I understand if you wouldn't want to automatically do that for every single selector. If given an option between #3836 and #3671, I would go with #3836. Correct if I'm wrong but couldn't we just do this:

import { categorySlice } from './categoriesSlice'

function CategoryList({ category }) {
  // Create a new memoized selector, for each component instance, on mount
  const selectItemsByCategory = useCallback(categorySlice.getSelectors(selectSlice).selectItemsByCategory, [])

  const itemsByCategory = useSelector(state =>
    selectItemsByCategory(state, category)
  )
}

Sorry if I'm sounding too disagreeable, I'm just trying to find the balance between provding good defaults and giving options when need to. And thank you @EskiMojo14 for the clarification. I'm probably not contributing much to the discussion, but I really appreciate you guys hearing me out.

[aryaemami59]

This is what it would look like:

[markerikson]

The biggest answer here is that while we do use Reselect in several places internally, we don't want to have a 100% restriction that you can only use Reselect here. For example, in the Redux docs we also recommend https://github.com/dai-shi/proxy-memoize as a useful selector library.

[aryaemami59]

I have actually used weakMapMemoize in production and it is awesome, I will post some direct comparisons in just a bit.

[markerikson]

FWIW, we're currently looking at punting this decision past RTK 2.0. We can release the current-in-beta createSlice.selectors field as-is, and consider any "selector factory" changes as a 2.x enhancement.