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

Add separate options for memoize and argMemoize, and merge together #608

Closed
wants to merge 1 commit into from
Closed

Add separate options for memoize and argMemoize, and merge together #608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 126 additions & 22 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,27 @@ function getDependencies(funcs: unknown[]) {
return dependencies as SelectorArray
}

export interface CreateSelectorOptions2<
/** Selectors will eventually accept some function to be memoized */
F extends (...args: unknown[]) => unknown,
MemoizeFunction extends (func: F, ...options: any[]) => F,
ArgsMemoizeFunction extends (
func: F,
...options: any[]
) => F = typeof defaultMemoize,
/** The additional options arguments to the memoizer */
MemoizeOptions extends unknown[] = DropFirst<Parameters<MemoizeFunction>>,
ArgsMemoizeOptions extends unknown[] = DropFirst<
Parameters<ArgsMemoizeFunction>
>
> {
memoize?: MemoizeFunction
memoizeOptions?: MemoizeOptions[0] | MemoizeOptions
argsMemoize?: ArgsMemoizeFunction
argsMemoizeOptions?: ArgsMemoizeOptions[0] | ArgsMemoizeOptions
}

// Legacy overload: `memoizeFunction` first
export function createSelectorCreator<
/** Selectors will eventually accept some function to be memoized */
F extends (...args: unknown[]) => unknown,
Expand All @@ -65,17 +86,72 @@ export function createSelectorCreator<
>(
memoize: MemoizeFunction,
...memoizeOptionsFromArgs: DropFirst<Parameters<MemoizeFunction>>
): CreateSelectorFunction<F, MemoizeFunction, typeof defaultMemoize>

export function createSelectorCreator<
/** Selectors will eventually accept some function to be memoized */
F extends (...args: unknown[]) => unknown,
MemoizeFunction extends (func: F, ...options: any[]) => F,
ArgsMemoizeFunction extends (
func: F,
...options: any[]
) => F = typeof defaultMemoize,
/** The additional options arguments to the memoizer */
MemoizeOptions extends unknown[] = DropFirst<Parameters<MemoizeFunction>>,
ArgsMemoizeOptions extends unknown[] = DropFirst<
Parameters<ArgsMemoizeFunction>
>
Comment on lines +99 to +103
Copy link
Contributor

Choose a reason for hiding this comment

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

From the pure TS perspective - this doesn't look quite correct. Those are either not generic (if you just want to unpack them from MemoizeFunction etc) or they are generic and MemoizeFunction etc should depend on them (as in: MemoizeFunction extends (func: F, ...options: MemoizeOptions) => F)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. My intent is to infer them here (and that's what it's already doing in 4.1). In other words:

  • Look at whatever memoize function was passed in
  • Determine the type of its arguments
  • Use those

I'm using generics with defaults here as a "precalculate this type once, ahead of time" step, because those get used 2-3 times later on in the file.

Copy link
Contributor

@Andarist Andarist May 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm using generics with defaults here as a "precalculate this type once, ahead of time" step, because those get used 2-3 times later on in the file.

That technique is fine-ish in isolated internal cases - and it can only give u some improved readability (since u can "alias" something), "precalculation" isn't really a thing that benefits u anyhow because TS implementation is heavily cached. If a type reference receives the same type arguments then the outcome will be the same type (in other words, internally Foo<{ arg: string }> is only ever evaluated once, even if you use it in thousand files).

From the pure TS perspective though - those aren't generic in the way you express it. You make this a part of the public API (not quite intentionally, u wanted private/local aliases), so you allow this:

createSelectorCreator<AnyFunction, typeof someMemoize, CompletelyUnrelated>(/*...*/)

>(
creatorOptions: CreateSelectorOptions2<
F,
MemoizeFunction,
ArgsMemoizeFunction
>
): CreateSelectorFunction<F, MemoizeFunction, ArgsMemoizeFunction>

export function createSelectorCreator<
F extends (...args: unknown[]) => unknown,
MemoizeFunction extends (func: F, ...options: any[]) => F,
ArgsMemoizeFunction extends (func: F, ...options: any[]) => F,
/** The additional options arguments to the memoizer */
MemoizeOptions extends unknown[] = DropFirst<Parameters<MemoizeFunction>>,
ArgsMemoizeOptions extends unknown[] = DropFirst<
Parameters<ArgsMemoizeFunction>
>
>(
memoizeOrOptions: MemoizeFunction | CreateSelectorOptions2<any, any, any>,
...memoizeOptionsFromArgs: unknown[]
) {
let createSelectorCreatorOptions: CreateSelectorOptions2<
F,
MemoizeFunction,
any
> = {
memoize: defaultMemoize as MemoizeFunction,
argsMemoize: defaultMemoize
}
if (typeof memoizeOrOptions === 'function') {
createSelectorCreatorOptions = {
...createSelectorCreatorOptions,
memoize: memoizeOrOptions,
memoizeOptions: memoizeOptionsFromArgs
}
} else {
createSelectorCreatorOptions = {
...createSelectorCreatorOptions,
...memoizeOrOptions
}
}

const createSelector = (...funcs: Function[]) => {
let recomputations = 0
let lastResult: unknown

// Due to the intricacies of rest params, we can't do an optional arg after `...funcs`.
// So, start by declaring the default value here.
// (And yes, the words 'memoize' and 'options' appear too many times in this next sequence.)
let directlyPassedOptions: CreateSelectorOptions<MemoizeOptions> = {
memoizeOptions: undefined
}
let directlyPassedOptions: Partial<CreateSelectorOptions2<any, any, any>> =
{}

// Normally, the result func or "output selector" is the last arg
let resultFunc = funcs.pop()
Expand All @@ -95,43 +171,70 @@ 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 combinedOptions = {
...createSelectorCreatorOptions,
...directlyPassedOptions
}

const {
memoize,
memoizeOptions = [],
argsMemoize,
argsMemoizeOptions = []
} = combinedOptions

// console.log('Options: ', { directlyPassedOptions, memoizeOptionsFromArgs })

// console.log('Selector options: ', createSelectorOptions)
const finalMemoizeOptions = ([] as unknown as MemoizeOptions).concat(
memoizeOptions
)

const finalArgsMemoizeOptions = (
[] as unknown as ArgsMemoizeOptions
).concat(argsMemoizeOptions)

// 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.
// Based on that, if `memoizeOptions` _is_ an array, we assume it's a full
// user-provided array of options. Otherwise, it must be just the _first_ arg, and so
// we wrap it in an array so we can apply it.
const finalMemoizeOptions = Array.isArray(memoizeOptions)
? memoizeOptions
: ([memoizeOptions] as MemoizeOptions)
// const finalMemoizeOptions = Array.isArray(memoizeOptions)
// ? memoizeOptions
// : ([memoizeOptions] as MemoizeOptions)

const dependencies = getDependencies(funcs)

const memoizedResultFunc = memoize(
function recomputationWrapper() {
recomputations++

// console.log('Recalculating results', { recomputations }, [...arguments])
// apply arguments instead of spreading for performance.
return resultFunc!.apply(null, arguments)
} as F,
...finalMemoizeOptions
)

// If a selector is called with the exact same arguments we don't need to traverse our dependencies again.
const selector = memoize(function dependenciesChecker() {
const params = []
const length = dependencies.length
const selector = argsMemoize(
function dependenciesChecker() {
// console.log('Recalculating inputs', [...arguments])
const params = []
const length = dependencies.length

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))
}
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))
}

// apply arguments instead of spreading for performance.
lastResult = memoizedResultFunc.apply(null, params)
return lastResult
} as F)
// apply arguments instead of spreading for performance.
lastResult = memoizedResultFunc.apply(null, params)
return lastResult
} as F,
...finalArgsMemoizeOptions
)

Object.assign(selector, {
resultFunc,
Expand All @@ -148,7 +251,7 @@ export function createSelectorCreator<
return createSelector as CreateSelectorFunction<
F,
MemoizeFunction,
MemoizeOptions
ArgsMemoizeFunction
>
}

Expand All @@ -162,6 +265,7 @@ export interface CreateSelectorOptions<MemoizeOptions extends unknown[]> {
export interface CreateSelectorFunction<
F extends (...args: unknown[]) => unknown,
MemoizeFunction extends (func: F, ...options: any[]) => F,
ArgsMemoizeFunction extends (func: F, ...options: any[]) => F,
MemoizeOptions extends unknown[] = DropFirst<Parameters<MemoizeFunction>>,
Keys = Expand<
Pick<ReturnType<MemoizeFunction>, keyof ReturnType<MemoizeFunction>>
Expand All @@ -187,12 +291,12 @@ export interface CreateSelectorFunction<
...items: [
...Selectors,
(...args: SelectorResultArray<Selectors>) => Result,
CreateSelectorOptions<MemoizeOptions>
CreateSelectorOptions2<F, MemoizeFunction, ArgsMemoizeFunction>
]
): OutputSelector<
Selectors,
Result,
((...args: SelectorResultArray<Selectors>) => Result),
(...args: SelectorResultArray<Selectors>) => Result,
GetParamsFromSelectors<Selectors>,
Keys
> &
Expand Down
21 changes: 21 additions & 0 deletions test/test_selector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,27 @@ describe('Customizing selectors', () => {

expect(memoizer3Calls).toBeGreaterThan(0)
})

test('createSelector accepts both memoize and argsMemoize', () => {
const selector = createSelector(
(state: StateAB) => state.a,
(state: StateAB) => state.b,
(a, b) => a + b,
{
memoize: lodashMemoize
}
)

expect(selector({ a: 1, b: 2 })).toBe(3)
expect(selector({ a: 1, b: 2 })).toBe(3)
expect(selector.recomputations()).toBe(1)
expect(selector({ a: 1, b: 3 })).toBe(4)
expect(selector.recomputations()).toBe(2)
expect(selector({ a: 1, b: 3 })).toBe(4)
expect(selector.recomputations()).toBe(2)
expect(selector({ a: 2, b: 3 })).toBe(5)
expect(selector.recomputations()).toBe(3)
})
})

describe('defaultMemoize', () => {
Expand Down