Skip to content

Conversation

@Methuselah96
Copy link
Collaborator

@Methuselah96 Methuselah96 commented Jul 26, 2020

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 docs to TypeScript, add definition files from the @types/react-select package, and then convert the source code to TypeScript little by little and removing the definition files as we went.

@changeset-bot
Copy link

changeset-bot bot commented Jul 26, 2020

💥 No Changeset

Latest 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 changesets

When 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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 26, 2020

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:

Sandbox Source
react-codesandboxer-example Configuration

@bladey bladey added pr/enhancement PRs that are specifically enhancing the project pr/needs-review PRs that need to be reviewed to determine outcome labels Aug 24, 2020
@Methuselah96 Methuselah96 added this to the 5.0 milestone Dec 12, 2020
overrides: [
{
files: ['*.ts', '*.tsx'],
extends: ['plugin:@typescript-eslint/base'],
Copy link

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:

https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/naming-convention.md#enforce-that-boolean-variables-are-prefixed-with-an-allowed-verb

This library in particular seems to have splitting behavior (isClearable / meniIsOpen)

Copy link
Collaborator Author

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.

Copy link

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.

@Methuselah96
Copy link
Collaborator Author

Closing in favor of a single PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr/enhancement PRs that are specifically enhancing the project pr/needs-review PRs that need to be reviewed to determine outcome

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants