- 
                Notifications
    You must be signed in to change notification settings 
- Fork 4.2k
Add TypeScript infrastructure #4139
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
Conversation
| 💥 No ChangesetLatest commit: 7195691 Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂 If these changes should be published to npm, you need to add a changeset. This PR includes no changesetsWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Click here to learn what changesets are, and how to add one. Click here if you're a maintainer who wants to add a changeset to this PR | 
| This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 7195691: 
 | 
e8698a6    to
    7195691      
    Compare
  
    | overrides: [ | ||
| { | ||
| files: ['*.ts', '*.tsx'], | ||
| extends: ['plugin:@typescript-eslint/base'], | 
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.
if we're making a breaking change for TS anyways, and are adding a linter. I wanted to recommend we had the following config for boolean naming of variables:
This library in particular seems to have splitting behavior (isClearable / meniIsOpen)
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.
This seems like mostly an issue of style. Not saying it would be a bad change, but that that we would need to discuss this as a team (as with any breaking change). I try to stay out of stylistic issues as much as possible (since I think the disagreement over them are often not worth the possible benefit of a better style). My personal opinion is that the pain of everyone having to change their code would be worse than the consistency benefits in this case.
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.
Certainly. To your point, just because it's a breaking change... doesn't mean we want every little bit of the API broken.
| Closing in favor of a single PR. | 
See #4142 for overall plan.
This PR starts adding the TypeScript infrastructure necessary in order to incrementally start transitioning to TypeScript. The idea would be to convert some of the
docsto TypeScript, add definition files from the@types/react-selectpackage, and then convert the source code to TypeScript little by little and removing the definition files as we went.