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

Fix: use unique value for NOT_FOUND #709

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

romgrk
Copy link

@romgrk romgrk commented Mar 15, 2024

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.

Copy link

netlify bot commented Mar 15, 2024

Deploy Preview for reselect-docs canceled.

Name Link
🔨 Latest commit 8ba87ff
🔍 Latest deploy log https://app.netlify.com/sites/reselect-docs/deploys/65fd8c92fcad270007313417

Copy link

codesandbox-ci bot commented Mar 15, 2024

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
Copy link
Contributor

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
Copy link
Author

romgrk commented Mar 22, 2024

I have added the suggested modifications.

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

Successfully merging this pull request may close these issues.

None yet

2 participants