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

react-redux: add types for hooks which will be added in version 7.1 #34913

Merged
merged 14 commits into from
Jun 12, 2019
Merged

Conversation

MrWolfZ
Copy link
Contributor

@MrWolfZ MrWolfZ commented Apr 22, 2019

Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

I am the author of the react-redux alpha hooks implementation (which I prototyped in TypeScript). These type definitions are mostly taken from my own source code.

@typescript-bot typescript-bot added the Popular package This PR affects a popular package (as counted by NPM download counts). label Apr 22, 2019
@typescript-bot
Copy link
Contributor

typescript-bot commented Apr 22, 2019

@MrWolfZ Thank you for submitting this PR!

🔔 @tkqubo @kenzierocks @clayne11 @tansongyang @NicholasBoll @mDibyo @Kallikrein @val1984 @jrakotoharisoa @apapirovski @surgeboris @soerenbf - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

* }
*/
export function useActions<
T extends [ActionCreator<A>, ActionCreator<A>, ActionCreator<A>, ActionCreator<A>, ActionCreator<A>, ActionCreator<A>, ActionCreator<A>, ActionCreator<A>, ActionCreator<A>],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still running tests locally, but we can likely replace all these overloads with a single T extends ReadonlyArray<readonly ActionCreator<A>>. That prevents the action creator tuple from getting widened to an array.

Copy link
Contributor

@the-dr-lazy the-dr-lazy Apr 22, 2019

Choose a reason for hiding this comment

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

Typescript couldn't infer the type of data in every index without tuples for now.

// With action creator tuples
useAction([a, b]) //=> [A, B]

// Without action creator tuples
useAction([a, b]) //=> (A | B)[]

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it only works properly on TypeScript 3.4 and above. Too bad, but it's a good option for a couple months from now...

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to release a major version that has a peer dependency on typescript@^3.4 now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compatibility with TypeScript 3.0 is exactly the reason why I typed it this way. I think these typings are just bearable enough that I wouldn't want to bump the peer dependency just for this. In a couple of months it's an easy fix.

@typescript-bot
Copy link
Contributor

typescript-bot commented Apr 22, 2019

@MrWolfZ The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

*/
export function useActions<
T extends [ActionCreator<A>, ActionCreator<A>, ActionCreator<A>, ActionCreator<A>, ActionCreator<A>, ActionCreator<A>, ActionCreator<A>, ActionCreator<A>, ActionCreator<A>],
A = AnyAction
Copy link
Contributor

@the-dr-lazy the-dr-lazy Apr 22, 2019

Choose a reason for hiding this comment

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

I think there is no need for the A = AnyAction. Actually, this generic type looks useless. Also, it causes problems with some Redux middlewares like redux-thunk. Maybe it is better to replace it with any.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I did it this way is to be similar to how bindActionCreators from redux is typed. I don't think it hurts to have that extra generic parameter since in pretty much all cases you are not going to specify any of the parameters anyways. However, as you say, it can easily be replaced with any or AnyAction, so if you still think I should remove it, I will do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's have a minimal implementation for it without extra generics. Also, it is important to don't restrict action creator return type to AnyAction for compatibility with thunks or other similar middlewares.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have merged the PR.

Copy link
Contributor

@threehams threehams Apr 23, 2019

Choose a reason for hiding this comment

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

I think we need feedback from more people on this. Redux bound actions are obnoxiously difficult to type since the return value is determined by the middleware you're running. We can't solve every case of this, but in the most basic case of this, setting this to any will cause a lot of errors since we can't do conditional checking between action creators (which return an object) and thunks (which return whatever the thunk returns).

The existing typings have built-in support for redux-thunk, for instance, which is missing from these types:

TActionCreator extends (
    ...args: infer TParams
  ) => (...args: any[]) => infer TReturn
    ? (...args: TParams) => TReturn
    : TActionCreator

Minimal example:

const thunkAction = () => () => {
  return { success: true };
}

const result = useActions(thunkAction);
  if (result.success) { // Property 'success' does not exist on type '() => () => { success: boolean; }'.
  }
}

@surgeboris and @rhys-vdw had to resolve a middleware-related bug/conflict in #30740, so they may have a specific interest here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll work on a PR tonight to allow any function as an action, but still support thunk return values. That should cover the simplest cases. It shouldn't be anywhere near as difficult as connect() has been.

I'll also add some tests so we're not guessing anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @threehams, just responding to the mention. Don't think I've got anything of value to add here as I haven't really dug into hooks yet, but thanks for the heads up. I'll keep an eye on the issue nonetheless.

@typescript-bot typescript-bot added Revision needed This PR needs code changes before it can be merged. and removed The Travis CI build failed labels Apr 22, 2019
@typescript-bot
Copy link
Contributor

@MrWolfZ One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

@typescript-bot
Copy link
Contributor

🔔 @thebrodmann - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

@typescript-bot
Copy link
Contributor

@MrWolfZ The Travis CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

export function useActions<T extends [ActionCreator<any>, ActionCreator<any>]>(actions: T, deps?: ReadonlyArray<any>): T;
export function useActions<T extends [ActionCreator<any>]>(actions: T, deps?: ReadonlyArray<any>): T;
export function useActions<T extends ReadonlyArray<ActionCreator<any>>>(actions: T, deps?: ReadonlyArray<any>): T;
export function useActions<T extends ActionCreatorsMapObject>(actions: T, deps?: ReadonlyArray<any>): T;
Copy link
Contributor

Choose a reason for hiding this comment

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

The object form has the same issue of not supporting thunks (or any random middleware-allowed action creator) as the other forms. I can take care of that tonight along with the other thunk changes.

@typescript-bot typescript-bot removed Other Approved This PR was reviewed and signed-off by a community member. Awaiting reviewer feedback labels Apr 23, 2019
@threehams
Copy link
Contributor

threehams commented Apr 24, 2019

Sorry, I have no idea how to do a pull request into your fork (doesn't seem to let me compare?)

This commit supports thunk return values, and adds test coverage for all hooks:
threehams@6459ed2

@MrWolfZ
Copy link
Contributor Author

MrWolfZ commented Apr 24, 2019

@threehams thanks, I have tried manually merging your changes into my fork. However, I get errors during linting:

ERROR: 1338:15  expect                   Compile error in typescript@3.0 but not in typescript@3.1.
Fix with a comment '// TypeScript Version: 3.1' just under the header.
Type 'ResolveArrayThunks<[(selected: boolean) => { type: string; payload: boolean; }, ({ items }: { items: string[]; }) => { type: string; payload: { items: string[]; }; }, () => (dispatch: Dispatch<AnyAction>) => { type: string; payload: never[]; }]>' is not an array type.
ERROR: 1354:15  expect                   Compile error in typescript@3.0 but not in typescript@3.1.
Fix with a comment '// TypeScript Version: 3.1' just under the header.
Type 'ResolveArrayThunks<[(selected: boolean) => { type: string; payload: boolean; }]>' is not an array type.
ERROR: 1462:13  expect                   Compile error in typescript@3.0 but not in typescript@3.1.
Fix with a comment '// TypeScript Version: 3.1' just under the header.
Type 'ResolveArrayThunks<[(selected: boolean) => { type: string; payload: boolean; }, ({ items }: { items: string[]; }) => { type: string; payload: { items: string[]; }; }, () => (dispatch: Dispatch<AnyAction>) => { type: string; payload: never[]; }]>' is not an array type.

I think the issue is with mapped types and tuples in 3.0. Therefore, we can either bump the TS version to 3.1, or we need to create a version of ResolveTupleThunks for each overload.

@MrWolfZ
Copy link
Contributor Author

MrWolfZ commented Apr 24, 2019

Oh, and another thing I noticed is that useDispatch doesn't allow dispatching thunks, but that is because the typing of Dispatch itself only allows actions to be dispatched. Any idea of how to work around this?


/**
* Compares two arbitrary values for shallow equality. Object values are compared based on their keys, i.e. they must
* have the same keys and for each key the value must be equal according to the `Object.is()` algorithm. Non-object

Choose a reason for hiding this comment

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

We're not actually using Object.is() or its equivalent right now - it's just a straight-up reference ===.

Copy link
Contributor Author

@MrWolfZ MrWolfZ May 26, 2019

Choose a reason for hiding this comment

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

@markerikson Are you maybe confusing this with something else? The shallowEqual function is using Object.is() as you can see here.

Choose a reason for hiding this comment

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

Whoops, yeah :)

@renchap
Copy link
Contributor

renchap commented Jun 3, 2019

Similar to how you can define you own useDispatch hook when you use redux-thunk to have proper typing, I think it would be useful to have an easy and documented way to have your own useSelector that you can export along your store and custom useDispatch, so you do not need to retype the state arg each time you use useSelector.

For example:

// Current version
import { useSelector } from "react-redux"
import { RootState } from "store"

const currentUser = useSelector((state: RootState) => state.currentUser)

// With proposal
import { useSelector } from "store"

const currentUser = useSelector(state => state.currentUser)

You can currently achieve this with something like:

import { useSelector as useSelectorBase } from "react-redux"

export const useSelector = <TSelected>(
  selector: (state: RootState) => TSelected,
  equalityFn?: (left: TSelected, right: TSelected) => boolean,
): TSelected => useSelectorOrig(selector, equalityFn)

which is a bit verbose because you need to repeat the TSelected template parameter. Would it be possible to have a more concise (and update-proof) way of typing your own useSelector?

@MrWolfZ
Copy link
Contributor Author

MrWolfZ commented Jun 4, 2019

@renchap I actually think that your example code isn't too bad, especially since you only need one of those in your app. However, what we could add something like this:

export interface TypedUseSelectorHook<TState> {
    <TSelected>(
        selector: (state: TState) => TSelected,
        equalityFn?: (left: TSelected, right: TSelected) => boolean
    ): TSelected
}

// use it like this
interface RootState {
  currentUser: CurrentUser;
}

const useTypedSelector: TypedUseSelectorHook<RootState> = useSelector;

What do you think?

@renchap
Copy link
Contributor

renchap commented Jun 5, 2019

@MrWolfZ Yes I has something like this in mind. For me there are 2 goals:

  • Have this documented somewhere, so people know they can use this pattern of custom hooks with pre-typed state (similar to useDispatch)
  • Be less dependent of the types implementation, so if the typings change you dont need to update your custom hook definition

Your proposal seems to solve both items :)

@MrWolfZ
Copy link
Contributor Author

MrWolfZ commented Jun 8, 2019

I've added the TypedUseSelectorHook interface to the type definitions. I've also added a paragraph to the useSelector comment that mentions this new interface. I am not sure if there is a better way of making people aware of its existence.

@typescript-bot
Copy link
Contributor

👋 Hi there! I’ve run some quick performance metrics against master and your PR. This is still an experiment, so don’t panic if I say something crazy! I’m still learning how to interpret these metrics.

Let’s review the numbers, shall we?

Comparison details 📊
master #34913 diff
Batch compilation
Memory usage 145944760.0 152659792.0 +4.6%
Type count 66229 66441 +0.3%
Assignability cache size 60423 60467 +0.1%
Subtype cache size 2523 2523 0.0%
Identity cache size 1152 1153 +0.1%
Language service
Samples taken 1583 1733 +9.5%
Identifiers in tests 1583 1733 +9.5%
getCompletionsAtPosition
    Mean duration (ms) 692.1 703.3 +1.6%
    Median duration (ms) 683.9 697.5 +2.0%
    Mean CV 5.7% 5.3% -6.2%
    Worst duration (ms) 919.3 951.6 +3.5%
    Worst identifier ConnectedWithOwnPropsStateless ConnectedWithOwnPropsClass
getQuickInfoAtPosition
    Mean duration (ms) 592.6 602.3 +1.6%
    Median duration (ms) 579.3 590.1 +1.9%
    Mean CV 6.4% 6.0% -6.1%
    Worst duration (ms) 870.0 900.1 +3.5%
    Worst identifier own TestUndefined

It looks like nothing changed too much. I’m pretty lenient since I’m still an experiment, so take a look anyways and make sure nothing looks out of place.


If you have any questions or comments about me, you can ping @andrewbranch. Have a nice day!

@RIP21
Copy link
Contributor

RIP21 commented Jun 11, 2019

@markerikson hey :) 7.1 got released, having this merged and published would be awesome :P

@markerikson
Copy link

@RIP21 : we don't control the typings at all :) that's up to the community.

@RIP21
Copy link
Contributor

RIP21 commented Jun 11, 2019

@markerikson Aha :P Gotcha. Saw you on reviewers list, thought that you are related to release somehow :)

Reviewers pleeeeease, make this one happen :)

@johnnyreilly johnnyreilly merged commit 6896243 into DefinitelyTyped:master Jun 12, 2019
@typescript-bot
Copy link
Contributor

I just published @types/react-redux@7.1.0 to npm.

iRON5 pushed a commit to iRON5/DefinitelyTyped that referenced this pull request Aug 13, 2019
…efinitelyTyped#34913)

* react-redux: add types for hooks which will be added in version 7.1

* react-redux: fix linting error due to redundant type declaration in JSDoc comment

* react-redux: convert action creator return type to any in useActions and useRedux

* react-redux: fix linting errors

* react-redux: fix mistake in `useActions` hook example code

* react-redux: add proper type annotations in hooks example code comments

* react-redux: support thunks in `useActions`, `useDispatch`, and `useRedux`; add tests for hooks; bump TypeScript required version to 3.1 (implemented by @threehams)

* react-redux: remove obsolete comment

* react-redux: adjust thunk array handling to work with TypeScript 3.0

* react-redux: Remove useRedux hook

With the release of react-redux@7.1.0-alpha.3, `useRedux` has been
removed, so removing it from the types.

* update typings for latest react-redux hooks API changes + add overload for `useDispatch` that allows customizing the type of the returned dispatch function

* react-redux: add equalityFn argument to useSelector

* react-redux: add newly exported shallowEqual function; add tests for shallowEqual and equalityFn parameter of useSelector

* react-redux: add interface that allows to easily create a typed useSelector hook
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Popular package This PR affects a popular package (as counted by NPM download counts). Revision needed This PR needs code changes before it can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.