-
Notifications
You must be signed in to change notification settings - Fork 270
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
Typescript Declaration #153
Conversation
@farskid, thanks for the PR, looking good so far! Regarding the
|
@rgommezz I'd try to upgrade my test react native typescript project to use react-redux v6 so I could test the integration as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution. So far looks good, but still got a few improvements to incorporate.
- Please double-check you're following best practices and update file header to follow a definitely-typed format (correct lib and TS version are especially important for consumers):
- http://definitelytyped.org/guides/contributing.html
- http://definitelytyped.org/guides/best-practices.html
-
Please add
types
property to the package.json referencing index.d.ts
https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html -
Also it would be good idea to incorporate at least few type-tests cases. Perhaps you could use this one as a reference and copy over some files: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react-redux/react-redux-tests.tsx
(Note: you would need to create a new folder with type-tests and put test examples inside with package.json, tsconfig and tslint copied from definitely typed reference, then we could run test script on CI or on push (whatever) to see if the types are not generating type-errors)
These type-tests would also be very useful for TS users as a reference of how to use this library with TypeScript.
All of that will make maintainers of types and the new contributors life easier.
@@ -0,0 +1,54 @@ | |||
import { Middleware, Reducer, AnyAction } from "redux"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redux should be included as a peer dependency with the appropriate version range, I don't see it right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate more on this? Redux is currently defined as devDependency of the project. Redux and React can be moved into peerDependencies of project as they're not truly a dependency but more a requirement. There would be no point of using this library without them already installed, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is what I have written, there is a missing peerDependency of redux and consumers need to know which redux version to use so types are working correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a bit tricky, since this library can be consumed with or without redux, for example if you just wanna use the React components utilities.
I don't see currently any option within npm to specify optional peerDependencies.
Still, if npm
only warns, we should go for the latest redux
version, which is 4.x.x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rgommezz what about this one, I think it should be fine:
I am putting the same package in both optionalDependencies and peerDependencies, and it seems to work. Not sure if this is the proper way or not.
@piotrwitek Thanks for the comments. I'll push the changes soon. |
@piotrwitek Do we need to have test file for definitions if we keep the declaration file inside the library? point is, if we don't publish types to DefinitelyTyped and instead keep it inside the library itself, would we still need to have tests? (considering that we don't have Typescript workflow in the repo yet). |
Yes, otherwise how would I know types are really working? (supported TS version, tsconfig options etc.) |
@piotrwitek I agree and see the point. I've checked a couple of famous libraries on npm around React and Redux. some of them use a simple setup of |
I know dtslint and I think we don't need it here. A simple setup with tsconfig and test file with some common use-case usage example should be good enough. |
Hey @farskid, any updates on this? As far as I understand, only some test cases are missing. I think the important ones are checking the interfaces for If you don't have much time at the moment, I can go ahead and take it from there if you want, since I acquired a bit of TS experience lately. Lemme know! |
@rgommezz I'll finish it up in a day or two :) |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Closing this issue after a prolonged period of inactivity. If this is still present in the latest release, please feel free to create a new issue with up-to-date information. |
Adds Typescript declaration file.
fixes: #131
It covers all the exported entities.