The Wayback Machine - https://web.archive.org/web/20230510075948/https://github.com/reduxjs/reselect/pull/608
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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
1 participant