reduxjs / reselect

Selector library for Redux
MIT License
19.04k stars 670 forks source link

Fix: use unique value for NOT_FOUND #709

Closed romgrk closed 5 months ago

romgrk commented 7 months ago

I'm not sure what's the reasoning for using a string constant for the not-found value, but an object feels both safer (real uniqueness, the string 'NOT_FOUND' could make the logic fail) and more performant (no strcmp() if both values are strings) which is handy when the selectors are called very often.

I've used a private class instance for the empty value because you seem to use the value type NOT_FOUND_TYPE, otherwise I would have gone with a Symbol('not-found') or an empty object, but any of these would be fine I think.

netlify[bot] commented 7 months ago

Deploy Preview for reselect-docs canceled.

Name Link
Latest commit 3692d5c47b1b2c4b52bbe901a40feb5dde85b7f1
Latest deploy log https://app.netlify.com/sites/reselect-docs/deploys/665b983d3138270008c9510f
codesandbox-ci[bot] commented 7 months ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

aryaemami59 commented 7 months ago

Seems like a good idea. Though I think in this particular scenario a export const NOT_FOUND = Symbol('NOT_FOUND') will be fine. And now if you add a test to lruMemoize.test.ts:

test('cache miss identifier does not collide with state values', () => {
    const state = ['NOT_FOUND', 'FOUND']

    type State = typeof state

    const createSelector = createSelectorCreator({
      memoize: lruMemoize,
      argsMemoize: lruMemoize
    }).withTypes<State>()

    const selector = createSelector(
      [(state, id: number) => state[id]],
      state => state,
      {
        argsMemoizeOptions: { maxSize: 10 },
        memoizeOptions: { maxSize: 10 }
      }
    )

    const firstResult = selector(state, 0)

    expect(selector(state, 1)).toBe(selector(state, 1))

    const secondResult = selector(state, 0)

    expect(secondResult).toBe('NOT_FOUND')

    expect(firstResult).toBe(secondResult)

    expect(selector.recomputations()).toBe(2)
  })

It would have failed previously as NOT_FOUND would cause a cache miss and selector.recomputations() would be 3 instead of 2. But now it should be good.

romgrk commented 7 months ago

I have added the suggested modifications.

romgrk commented 5 months ago

Ping, but feel free to close the PR if you're not going to merge it.

aryaemami59 commented 5 months ago

@romgrk Maintainers are a little busy at the moment, I'm sure they will look at it as soon as they get a chance. In the meantime, if you don't mind, could you update the code with the provided suggestions?

markerikson commented 5 months ago

Thanks!