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

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

Closed
wants to merge 1 commit into from

Conversation

markerikson
Copy link
Contributor

@markerikson markerikson commented Apr 27, 2023

This PR:

  • Attempts to modify createSelectorCreator to accept either the existing signature of (memoizeFunction, ...memoizeOptions), or the new signature of (options: CreateSelectorOptions) (which includes {memoize, memoizeOptions, argMemoize, argMemoizeOptions})
  • Updates createSelector to merge together the options provided to createSelectorCreator, with whatever options were passed in directly as an argument

The initial logic appears to work okay at the JS level, as the tests pass. But, the types aren't right. As soon as I do this:

    const selector = createSelector(
      (state: StateAB) => state.a,
      (state: StateAB) => state.b,
      (a, b) => a + b,
      {
        memoize: lodashMemoize
      }
    )

a and b lose their types and it yells at me.

Details

I strongly suspect that what I want to do runtime-wise is just too dynamic to express in TS very well.

The general idea is:

  • Start with the memoize function given to createSelectorCreator. This accepts some kind of memoizeOptions (like an equality function), so allow passing in {memoize, memoizeOptions}
  • However, createSelector would later allow overriding those on a per-call basis, like: createSelector(in1, in2, output, {memoize: lodashMemoize, memoizeOptions: lodashMemoizeOptionsHere}).

where it gets even screwer is that maybe you'd pass in memoizeOptions by itself and have that apply to the existing memoize function, or pass in both {memoize, memoizeOptions} and have it apply to the new memoize function

Runtime-wise, this is basically just an {...createSelectorCreatorOptions, ...createSelectorDirectOptions}.

And then to make it even more complicated, I want to allow doing the same thing for argMemoize and argMemoizeOptions. (although I imagine if I could get the types right for one function I can get it right for both)

I have a vague feeling that I somehow need to allow for passing multiple generics for, say, MemoizeFunction1 and MemoizeFunction2 and doing something like type FinalMemoizeFunction = MemoizeFunction2 extends unknown ? MemoizeFunction2 : MemoizeFunction1. But there's enough complexity here my brain is shutting down trying to think about it.

@codesandbox-ci
Copy link

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.

Latest deployment of this branch, based on commit 1c88935:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration

Comment on lines +99 to +103
/** The additional options arguments to the memoizer */
MemoizeOptions extends unknown[] = DropFirst<Parameters<MemoizeFunction>>,
ArgsMemoizeOptions extends unknown[] = DropFirst<
Parameters<ArgsMemoizeFunction>
>
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

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>(/*...*/)

@markerikson
Copy link
Contributor Author

Superseded by #626 !

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.

2 participants