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

Update TypeScript declarations to work with Redux 3 & 4 #7

Closed
xuoe opened this issue Nov 13, 2019 · 5 comments
Closed

Update TypeScript declarations to work with Redux 3 & 4 #7

xuoe opened this issue Nov 13, 2019 · 5 comments

Comments

@xuoe
Copy link
Owner

xuoe commented Nov 13, 2019

For more details, see PR #6.

@KagamiChan
Copy link

KagamiChan commented Nov 14, 2019

The main reason to update the TS declaration is that Redux has been rewritten with TypeScript, so the @types/redux is no longer required and we see some incompat in both declarations

(Off topic, are you interested in doing similar rewrite?)

The suggestions from @ankon are excellent. As Dispatcher is an internal typing that user will not touch, redux 3/4 users won't notice any difference.

But for those who use pre-TS redux, if we drop @types/redux dependency, their code would break because they have to upgrade redux, for this reason it is going to be a breaking change.

So we need to drop the @types/redux dep, add a redux@4 peer dep, and bump a major version

Besides, in TS Redux definition, Dispatch accepts an Action generics, we could consider adding an extra argument for that, so the additional reload could be:

import { Action } from 'redux'

type Dispatcher<MS, A> = (dispatch: Dispatch<A>, currentState: MS, previousState: MS) => unknown;
// or no reload, just type Dispatcher<MS, A extends Action = AnyAction> = (dispatch: Dispatch<A>, currentState: MS, previousState: MS) => unknown;

export function observer<S, MS, A extends Action>(mapper: Mapper<S, MS>, dispatcher: Dispatcher<MS, A>, options?: Options): Observer<S, MS>;

Please correct me in case of any error @ankon

@KagamiChan
Copy link

and also, we need to add some TypeScript type checking to make sure the declaration works with some concrete example

xuoe pushed a commit that referenced this issue Nov 14, 2019
@xuoe
Copy link
Owner Author

xuoe commented Nov 14, 2019

Thanks a lot for your input, @KagamiChan.

I created a pull request with some changes to the TypeScript declarations (#8) and added some comments there. Please take a look, if you have the time.

I should mention upfront that I haven't written any TypeScript before today and I haven't used this package (or Redux) in years, so I'm learning as we're going.

Regarding @types/redux: removing it from package.json and reinstalling all development dependencies and then issuing tsc --strict --noImplicitAny --noEmit ./index.d.ts does not generate any errors with either version of Redux (3.7.2 or 4.0.4). With 3.7.2, there's even a deprecation warning issued at install time, which states that @types/redux is unnecessary since redux comes with its own declaration file. I tried this even against older versions that didn't have a declaration file and it still worked, which is confusing (I don't have @types/redux installed globally). It seems that 3.4.0 is the first version that supports type declarations, and it's been released almost 4 years ago. TypeScript users that use older versions of Redux could fix the problem by tagging @types/redux as their own dependency, which they probably already have done, but that's a lot of assumptions.

Regarding a rewrite: do you mean a rewrite in TypeScript? No, I don't think it's necessary. Getting the declarations to work correctly would be more than enough, in my opinion.

Regarding your code suggestions: I did include something similar in #8 (f13a2a4). What do you make of it?

@KagamiChan
Copy link

KagamiChan commented Nov 14, 2019

Thanks for your quick response

The type checking I mentioned is write some TS code to consume the package in various way and see if there's no conplaints from tsc, Maybe you could take some examples from tests, or I prepare some later when I have bandwidth (my project consuming the package is not in TS so I don't have any example at hand).

Sorry, I forgot that they have shipped with typings before TS rewrite. They should have picked the dts file from definitivelyTyped repo, and I see that currently @types/redux depends on redux and exports the typings from redux. Then it seems simpler, things making difference is just the typing change between version 3 and 4

@xuoe
Copy link
Owner Author

xuoe commented Nov 14, 2019

Yes, I'm writing some tests to ensure that the type declarations work with >3.4.x (as described on the dtslint README). The typing issue appears to be solved by using Dispatch<AnyAction> instead of Dispatch<MS> (with AnyAction being the type exported by redux).

I'll take a look at your comments from #8 a bit later. Thanks!

@xuoe xuoe closed this as completed in 8c66be3 Nov 15, 2019
xuoe added a commit that referenced this issue Nov 5, 2023
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

No branches or pull requests

2 participants