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

Convert to TypeScript #4142

Closed
Methuselah96 opened this issue Jul 27, 2020 · 13 comments · Fixed by #4489
Closed

Convert to TypeScript #4142

Methuselah96 opened this issue Jul 27, 2020 · 13 comments · Fixed by #4489
Labels
issue/reviewed Issue has recently been reviewed (mid-2020)
Milestone

Comments

@Methuselah96
Copy link
Collaborator

Methuselah96 commented Jul 27, 2020

This is an issue to track the progress of converting this project to TypeScript. The plan is to do this incrementally in order to make the transition as smooth as possible and not make the review process too overwhelming. The overall plan is:

  1. Incrementally convert docs to TypeScript.
  1. Move Typescript declaration files from @types/react-select to this repository.
  2. Incrementally convert main source files to TypeScript.
@bladey
Copy link
Contributor

bladey commented Aug 24, 2020

Thank you for all your efforts @Methuselah96, this looks great.

@Methuselah96 Methuselah96 added this to the 5.0 milestone Dec 12, 2020
@Methuselah96
Copy link
Collaborator Author

Methuselah96 commented Dec 15, 2020

@kylemh Thanks for all your help so far on PRs! I saw on #4293 that you're looking to help out with the TS conversion. Most of the work is actually already done. (I have the entire code-base converted to TypeScript on a branch, it's just a matter of figuring out how best to merge it in.)

The PRs I've made so far have been to incrementally convert the docs to TypeScript using the types that currently exist on DefinitelyTyped. When I run across a problem in the types, I make a PR to DefinitelyTyped. This works, however it's slow because there are no active reviewers for react-select on DefinitelyTyped. Would be willing to update the types on DefinitelyTyped to 3.1 (by adding the isLoading prop to AsyncSelect) and also add yourself as a definition author? That way we can review each others PRs on DefinitelyTyped and this process can be sped up quite a bit.

Thanks for all your hard work!

@kylemh
Copy link

kylemh commented Dec 15, 2020

Down; however, that looks to already be done, no?

@Methuselah96
Copy link
Collaborator Author

Methuselah96 commented Dec 15, 2020

I think it's in the State, but not in the Props.

@kylemh
Copy link

kylemh commented Dec 15, 2020

Async extends State. Async represents the inferred props on the default export of /async:

Screen Shot 2020-12-15 at 10 36 08 AM

@kylemh
Copy link

kylemh commented Dec 15, 2020

I did reference the wrong line of code though. The AsyncSelect props extends the "NamedProps" interface -> https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react-select/src/Select.d.ts#L215 which has isLoading

@Methuselah96
Copy link
Collaborator Author

Methuselah96 commented Dec 15, 2020

I see now, the PR that added the isLoading prop to Async just made it so that it was passed through the Select properly. My bad. Can you just make a PR to bump the version number to 3.1 and add yourself as an author? Or you can find an actual change to make; there are quite a few of missing/incorrect types (for example, tightening up these styles types so they don't pass any as the props).

@kylemh
Copy link

kylemh commented Dec 15, 2020

Certainly! My work week is a bit busy, but I'll come back to this at some point next week at the worst.

@Methuselah96
Copy link
Collaborator Author

No worries, I can relate. Just whenever you find the time.

@kylemh
Copy link

kylemh commented Dec 21, 2020

I'll make changes to the styles type you mentioned by EOD tomorrow (my guess is you mean to remove the TODO comments and add generics to the style functions so that the props for each style fn are properly typed).

By "bump the version to 3.1", you mean for: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react-select/index.d.ts#L1

Additionally, did we want to upgrade the minimum required version of TS to expand what we can use?

@Methuselah96
Copy link
Collaborator Author

Methuselah96 commented Dec 21, 2020

Thanks! Yeah, you got it. And don't forget to add yourself as a definition owner as well.

Regarding the minimum required version: Can you just remove the // TypeScript version 2.9 that currently exists? DefinitelyTyped by default sets the minimum version to any TypeScript version less than 2 years old, which is 3.3 right now. I'd like to try to stick with that default minimum version if possible unless there's a compelling reason why we need to use newer versions.

@kylemh
Copy link

kylemh commented Dec 21, 2020

TIL! On it.

@kylemh
Copy link

kylemh commented Dec 22, 2020

I'd like follow up with a PR after this one which simply adjusts comments (like these) to JS Doc comments. It seems so minor, but VS Code users will begin to see intellisense and - cooler still - tools like react-docgen-typescript can parse JS Doc comments into markdown for tools like Storybook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue/reviewed Issue has recently been reviewed (mid-2020)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants