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.

Sign up for GitHub

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

Jump to bottom

Run input selectors twice, to check stability. #612

Merged
merged 12 commits into from May 14, 2023
Merged

Run input selectors twice, to check stability. #612

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
74331cb
Begin investigating running input selectors twice, to check stability.
EskiMojo14 May 13, 2023
ecfb455
move entire check inside single if block
EskiMojo14 May 13, 2023
6431753
run performance checks with production settings
EskiMojo14 May 13, 2023
903b463
add some tests
EskiMojo14 May 13, 2023
f1f6fef
use a library-wide flag instead of per-selector config
EskiMojo14 May 13, 2023
db2c621
more intuitive naming
EskiMojo14 May 13, 2023
e4102b6
use provided memoize for stability check (and fix flag)
EskiMojo14 May 13, 2023
ebcde3c
add per selector option back, and support running check once
EskiMojo14 May 14, 2023
1e597af
default check to "once" instead of always
EskiMojo14 May 14, 2023
eb7b5fd
add to README
EskiMojo14 May 14, 2023
a163264
update table of contents
EskiMojo14 May 14, 2023
afc8bd3
use an enum, and log arguments and inputs
EskiMojo14 May 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 32 additions & 2 deletions src/index.ts
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@ export type {
} from './types'

import {
createCacheKeyComparator,
defaultMemoize,
defaultEqualityCheck,
DefaultMemoizeOptions
Expand Down Expand Up @@ -98,7 +99,10 @@ export function createSelectorCreator<

// Determine which set of options we're using. Prefer options passed directly,
// but fall back to options given to createSelectorCreator.
const { memoizeOptions = memoizeOptionsFromArgs } = directlyPassedOptions
const {
memoizeOptions = memoizeOptionsFromArgs,
inputStabilityCheck = true
} = directlyPassedOptions

// Simplifying assumption: it's unlikely that the first options arg of the provided memoizer
// is an array. In most libs I've looked at, it's an equality function or options object.
Expand All @@ -111,6 +115,9 @@ export function createSelectorCreator<

const dependencies = getDependencies(funcs)

// do we want to try and use the equality fn from options? how do we account for other memoizers?
const cacheKeyComparator = createCacheKeyComparator(defaultEqualityCheck)
EskiMojo14 marked this conversation as resolved.
Show resolved Hide resolved

const memoizedResultFunc = memoize(
function recomputationWrapper() {
recomputations++
Expand All @@ -133,10 +140,32 @@ export function createSelectorCreator<
const params = []
const length = dependencies.length

const paramsCopy = []

for (let i = 0; i < length; i++) {
// apply arguments instead of spreading and mutate a local list of params for performance.
// @ts-ignore
params.push(dependencies[i].apply(null, arguments))

if (process.env.NODE_ENV !== 'production' && inputStabilityCheck) {
EskiMojo14 marked this conversation as resolved.
Show resolved Hide resolved
// make a second copy of the params, to check if we got the same results
// @ts-ignore
paramsCopy.push(dependencies[i].apply(null, arguments))
}
}

if (process.env.NODE_ENV !== 'production' && inputStabilityCheck) {
const equal = cacheKeyComparator(params, paramsCopy)
if (!equal) {
// do we want to log more information about the selector?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 What happens if we reference selector.name here? Does it end up as "selector", or whatever we assigned it to?

Scratch that, it comes back as "memoized", because defaultMemoize.ts has function memoized().

Mmm... We could at least log out the original inputs and both sets of extracted values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

{
  arguments,
  firstInputs: params,
  secondInputs: paramsCopy
}

open to better names 🙂

console.warn(
`An input selector returned a different result when passed same arguments.
This means your output selector will likely run more frequently than intended.
Avoid returning a new reference inside your input selector, e.g.
\`createSelector([(arg1, arg2) => ({ arg1, arg2 })],(arg1, arg2) => {})\`
`
)
}
}

// apply arguments instead of spreading for performance.
Expand Down Expand Up @@ -164,7 +193,8 @@ export function createSelectorCreator<
}

export interface CreateSelectorOptions<MemoizeOptions extends unknown[]> {
memoizeOptions: MemoizeOptions[0] | MemoizeOptions
memoizeOptions?: MemoizeOptions[0] | MemoizeOptions
inputStabilityCheck?: boolean
}

/**
Expand Down