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

Typed createSelector function-solution-1 #624

Closed

Conversation

aryaemami59
Copy link
Contributor

@aryaemami59 aryaemami59 commented Sep 14, 2023

Hey folks.

So I was digging through the codebase and I thought I could provide some help, the first thing I thought I could do is providing an option to create a pre-typed createSelector function that is typed for the RootState much like TypedUseSelectorHook. I came up with a couple of potential solutions. This is the first solution:

export interface CreateSelectorFunction<
  State,
  MemoizeFunction extends (
    func: (...args: unknown[]) => unknown,
    ...options: any[]
  ) => (...args: unknown[]) => unknown,
  MemoizeOptions extends unknown[] = DropFirst<Parameters<MemoizeFunction>>,
  Keys = Expand<
    Pick<ReturnType<MemoizeFunction>, keyof ReturnType<MemoizeFunction>>
  >
> {
  /** Input selectors as separate inline arguments */
  <Selectors extends readonly Selector<State>[], Result>(
    ...items: [
      ...Selectors,
      (...args: SelectorResultArray<Selectors>) => Result
    ]
  ): OutputSelector<
    Selectors,
    Result,
    (...args: SelectorResultArray<Selectors>) => Result,
    GetParamsFromSelectors<Selectors>,
    Keys
  > &
    Keys

  /** Input selectors as separate inline arguments with memoizeOptions passed */
  <Selectors extends readonly Selector<State>[], Result>(
    ...items: [
      ...Selectors,
      (...args: SelectorResultArray<Selectors>) => Result,
      CreateSelectorOptions<MemoizeOptions>
    ]
  ): OutputSelector<
    Selectors,
    Result,
    (...args: SelectorResultArray<Selectors>) => Result,
    GetParamsFromSelectors<Selectors>,
    Keys
  > &
    Keys

  /** Input selectors as a separate array */
  <Selectors extends readonly Selector<State>[], Result>(
    selectors: [...Selectors],
    combiner: (...args: SelectorResultArray<Selectors>) => Result,
    options?: CreateSelectorOptions<MemoizeOptions>
  ): OutputSelector<
    Selectors,
    Result,
    (...args: SelectorResultArray<Selectors>) => Result,
    GetParamsFromSelectors<Selectors>,
    Keys
  > &
    Keys
}

We can also rename it to TypedCreateSelectorFunction.

export function createSelectorCreator<
  /** A memoizer such as defaultMemoize that accepts a function + some possible options */
  MemoizeFunction extends (
    func: (...args: unknown[]) => unknown,
    ...options: any[]
  ) => (...args: unknown[]) => unknown,
  /** The additional options arguments to the memoizer */
  MemoizeOptions extends unknown[] = DropFirst<Parameters<MemoizeFunction>>
>(
  memoize: MemoizeFunction,
  ...memoizeOptionsFromArgs: DropFirst<Parameters<MemoizeFunction>>
) {
  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> = {}

    // Normally, the result func or "output selector" is the last arg
    let resultFunc = funcs.pop()

    // If the result func is actually an _object_, assume it's our options object
    if (typeof resultFunc === 'object') {
      directlyPassedOptions = resultFunc as any
      // and pop the real result func off
      resultFunc = funcs.pop()
    }

    if (typeof resultFunc !== 'function') {
      throw new Error(
        `createSelector expects an output function after the inputs, but received: [${typeof resultFunc}]`
      )
    }

    // Determine which set of options we're using. Prefer options passed directly,
    // but fall back to options given to createSelectorCreator.
    const { memoizeOptions = memoizeOptionsFromArgs, inputStabilityCheck } =
      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.
    // 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 dependencies = getDependencies(funcs)

    const memoizedResultFunc = memoize(
      function recomputationWrapper() {
        recomputations++
        // apply arguments instead of spreading for performance.
        return resultFunc!.apply(null, arguments)
      } as Parameters<MemoizeFunction>[0],
      ...finalMemoizeOptions
    )

    // @ts-ignore
    const makeAnObject: (...args: unknown[]) => object = memoize(
      // @ts-ignore
      () => ({}),
      ...finalMemoizeOptions
    )

    let firstRun = true

    // If a selector is called with the exact same arguments we don't need to traverse our dependencies again.
    // TODO This was changed to `memoize` in 4.0.0 ( #297 ), but I changed it back.
    // The original intent was to allow customizing things like skortchmark's
    // selector debugging setup.
    // But, there's multiple issues:
    // - We don't pass in `memoizeOptions`
    // Arguments change all the time, but input values change less often.
    // Most of the time shallow equality _is_ what we really want here.
    // TODO Rethink this change, or find a way to expose more options?
    const selector = defaultMemoize(function dependenciesChecker() {
      const params = []
      const { length } = dependencies

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

      const finalStabilityCheck = inputStabilityCheck ?? globalStabilityCheck

      if (
        process.env.NODE_ENV !== 'production' &&
        (finalStabilityCheck === 'always' ||
          (finalStabilityCheck === 'once' && firstRun))
      ) {
        const paramsCopy = []

        for (let i = 0; i < length; i++) {
          // 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 the memoize method thinks the parameters are equal, these *should* be the same reference
        const equal =
          makeAnObject.apply(null, params) ===
          makeAnObject.apply(null, paramsCopy)
        if (!equal) {
          // do we want to log more information about the selector?
          console.warn(
            'An input selector returned a different result when passed same arguments.' +
              '\nThis means your output selector will likely run more frequently than intended.' +
              '\nAvoid returning a new reference inside your input selector, e.g.' +
              '\n`createSelector([(arg1, arg2) => ({ arg1, arg2 })],(arg1, arg2) => {})`',
            {
              arguments,
              firstInputs: params,
              secondInputs: paramsCopy
            }
          )
        }

        if (firstRun) firstRun = false
      }

      // apply arguments instead of spreading for performance.
      lastResult = memoizedResultFunc.apply(null, params)

      return lastResult
    } as Parameters<MemoizeFunction>[0])

    Object.assign(selector, {
      resultFunc,
      memoizedResultFunc,
      dependencies,
      lastResult: () => lastResult,
      recomputations: () => recomputations,
      resetRecomputations: () => (recomputations = 0)
    })

    return selector
  }
  // @ts-ignore
  return createSelector as CreateSelectorFunction<
    any,
    MemoizeFunction,
    MemoizeOptions
  >
}

I will make a separate PR for the second solution.

… a generic. this solution also slightly simplifies the types for CreateSelectorFunction and createSelectorCreator
@aryaemami59 aryaemami59 marked this pull request as ready for review September 14, 2023 19:01
@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 af855be:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration

@markerikson
Copy link
Contributor

@aryaemami59 : now that #626 is merged, do we still need either of these PRs?

@aryaemami59
Copy link
Contributor Author

@aryaemami59 : now that #626 is merged, do we still need either of these PRs?

I will go ahead and close this one, and #625 since they are no longer applicable, will probably come up with a new solution on a type level or include a copy paste pattern in the docs.

@aryaemami59 aryaemami59 deleted the TypedCreateSelector-solution-1 branch February 8, 2024 11:35
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