reduxjs/reselect

Duplication Result parameter in generics

AlbertFib opened this issue · 9 comments

Hi!
We are actively using ‘reselect’ on the project.
Sometimes you need to explicitly specify the type of selector created using createSelector for contract validation. To do this, we use the OutputSelector and OutputParametricSelector types.
In the definition of types, it is necessary to duplicate information about the result (as a separate field and as the returned type of the combiner). Why not deduce the type of result from the definition of the combiner and remove duplication?

For example:

export type OutputSelector<S extends SelectorArray, Combiner extends UnknownFunction, Params extends readonly any[] = never, Keys = {}>
   = Selector<GetStateFromSelectors<S>, ReturnType<Combiner>, Params> & OutputSelectorFields<Combiner, Keys>

export type OutputParametricSelector<State, Props, Combiner extends UnknownFunction, Keys = {}> =
    ParametricSelector<State, Props, ReturnType<Combiner>> & OutputSelectorFields<Combiner, Keys>;

I'm kinda confused. Can you give an example of when you feel you need to explicitly specify the type?

Reselect's current types are designed to infer as much as possible, and you generally shouldn't be explicitly providing generic arguments to createSelector.

At the moment, we have recently migrated a large project from flow to typescript and the typing of the code leaves much to be desired. Automatic type inference often outputs any. By explicitly specifying the types for the combiner, we also fix the composition of selectors inside createSelector

You're supposed to use Reselect the other way around :)

Type the input function arguments correctly, and everything else should get inferred right.

In this case, go back to the definitions of selectCheckoutCartGroups, selectCheckoutCarts , etc, and make sure that their input functions have the right types, like:

const selectCheckoutCartGroups = createSelector(
  (state: RootState) => state.some.cart,
  (state: RootState, someOtherArg: number) => someOtherArg,
  (cart, someOtherArg) => // whatever here
)

// selectCheckoutCartGroups is now typed as;
// (state: RootState, someOtherArg: number) => whateverYourReturnValueIs

Do that for each of those selectors, and the types will propagate through correctly.

While we're at it, what is this stateCreator function?

While we're at it, what is this stateCreator function?

Probably a bad example
This is a legacy code, there is nothing good there, unfortunately :)

You're supposed to use Reselect the other way around :)

Type the input function arguments correctly, and everything else should get inferred right.

In this case, go back to the definitions of selectCheckoutCartGroups, selectCheckoutCarts , etc, and make sure that their input functions have the right types, like:

const selectCheckoutCartGroups = createSelector(
  (state: RootState) => state.some.cart,
  (state: RootState, someOtherArg: number) => someOtherArg,
  (cart, someOtherArg) => // whatever here
)

// selectCheckoutCartGroups is now typed as;
// (state: RootState, someOtherArg: number) => whateverYourReturnValueIs

Do that for each of those selectors, and the types will propagate through correctly.

Yes, this is the ideal option that we are striving for. Unfortunately, now at the moment there is a need for part of the team to type explicitly. Even the described types do not always give the expected result. To increase readability, we would like to remove an extra parameter that is duplicated in the type. Of the options was to redefine them locally or come here with a suggestion :)

@AlbertFib : why do you need to "type explicitly"?

@AlbertFib : why do you need to "type explicitly"?

In some places we consciously want to type explicitly and control the return type of the created selector, and not rely on automatic output, so that if the selector changes, we get a type error in this place.

Honestly, you're bringing the pain upon yourself :)

We designed Reselect's types to infer as much as possible, so that you don't have to write all the types out explicitly.

Given that, I don't think we're going to make a change here.