-
Notifications
You must be signed in to change notification settings - Fork 671
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
Conversation
✅ Deploy Preview for reselect-docs canceled.
|
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. |
Seems like a good idea. Though I think in this particular scenario a 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 |
I have added the suggested modifications. |
Ping, but feel free to close the PR if you're not going to merge it. |
@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? |
Co-authored-by: Arya Emami <aryaemami59@yahoo.com>
Thanks! |
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 (nostrcmp()
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 aSymbol('not-found')
or an empty object, but any of these would be fine I think.