Skip to content

Add Typescript definitions. #433

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

Closed
wants to merge 1 commit into from
Closed

Add Typescript definitions. #433

wants to merge 1 commit into from

Conversation

DaanDeMeyer
Copy link

Having Typescript definitions in the repository itself saves users from having to download the definitions separately and changes to the main redux definitions can be more easily applied to the react-redux definitions.

I've taken the definitions from the DefinitelyTyped repository. The only change I made is the addition of the generic State type to the connect function so the State parameter of the mapStateToProps function can actually be typed instead of being of the 'any' type.

The rest of the definitions seem to be correct.

@DaanDeMeyer DaanDeMeyer changed the title Added Typescript definitions. Add Typescript definitions. Jul 12, 2016
@timdorr
Copy link
Member

timdorr commented Aug 11, 2016

Any chance we can get testing for these, like we do on Redux? https://github.com/reactjs/redux/blob/master/test/typescript.spec.js

@timdorr timdorr mentioned this pull request Aug 27, 2016
7 tasks
@Guria
Copy link

Guria commented Sep 1, 2016

This typings provided doesn't fit to be bundled with package because the are global. You must convert it to be external module definitions (using import, export keywords, not using global namespace imports, etc)

@bbenezech
Copy link

this.props.dispatch is missing in connect() and connect(mapStateToProps) forms.

@DaanDeMeyer
Copy link
Author

@timdorr Do the Typescript definitions have to take into account that the library can be used as an UMD module? Also, can I make use of the Typescript 2.0 features for the definitions?

@bbenezech I don't see what's missing. Could you explain more in depth?

@bbenezech
Copy link

bbenezech commented Sep 6, 2016

@DaanDeMeyer Sorry, disregard.
I overlooked connect<TOwnProps, TStateProps, TDispatchProps>.
I can use TDispatchProps and inform my component that it has a dispatch prop passed from its HOC.

Really sorry. Still trying to wrap my head with TS

@DaanDeMeyer
Copy link
Author

Since I'm not using react-redux anymore I'm not going to be able to implement testing or maintain definitions for the project so I'm closing the pull request.

@Zalastax
Copy link

@codeandcats suggested I fork and re-request these additions. Before I start, I wonder what are the tasks that need to be performed? I don't mind opening a PR but I'd rather not just copy the code without adding anything myself, unless we are content with the state of this PR before it was closed.
@DaanDeMeyer raised some questions which I'd like to get answered before I begin.

  • @timdorr Do the Typescript definitions have to take into account that the library can be used as an UMD module? Also, can I make use of the Typescript 2.0 features for the definitions?
  • The rest of the definitions seem to be correct. Anything missing? Probably needs to be converted to external module though.
  • Do we have any typescript files that can be used to test this with typescript-definition-tester?

@codeandcats
Copy link

@timdorr _bump_

Also, imho it would be fine to target TypeScript 2 since it's backwards compatible with 1.x code so there's really no reason consumers shouldn't be using it now.

@timdorr
Copy link
Member

timdorr commented Nov 7, 2016

Since MS is pushing the whole @types namespacing on npm, I don't think we need to add them here. They exist on DT and that should be good enough.

@bbenezech
Copy link

@timdorr
Redux typings and flow typings are successfully maintained in-house, why not react-redux?
DT is a gigantic mess, noise there is overwhelming. It makes sense to have it maintained here, with PR merged by react-redux maintainers, unless you do want to reduce the load on the library's maintenance, which is very understandable.

@timdorr
Copy link
Member

timdorr commented Nov 7, 2016

It's more that our current set of maintainers don't use TS, so we don't have any ability to maintain those typings. If you want to see it here, I'm more than happy to accept a PR, but understand that we'll rely on the community to guide those typings. That's how we did the TS and Flow typings on Redux.

I can basically just read through comments and mash the appropriate button. I'm a typing dunce 😄

@Guria
Copy link

Guria commented Nov 7, 2016

Since MS is pushing the whole @types namespacing on npm, I don't think we need to add them here

It is not true. @types repos is a fallback for libs that doesn't maintain typings in house. It also introduces compatibility issues.
In house typings still prefered way, because lives near source code and shares the same version.
Of course there is a cost for it too:

  • increased maintainance load
  • it affects semver: breaking change in typings would require major bump in whole package

But again it is much more convinient for consumer to use lib with typings included:

  • no additional deps to install and maintain
  • typings always describe code that you use since it located at the same package
  • it usually have higher quality than 3rd parties

@jimbolla
Copy link
Contributor

jimbolla commented Nov 7, 2016

I think the optimal situation is to have Flow/TS typings collocated with the code, but have at least 1 project maintainer committed to keeping them updated in step with code changes.

@bbenezech bbenezech mentioned this pull request Nov 7, 2016
@bbenezech
Copy link

Ok, let's try. Go easy on me #538

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.

7 participants