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

feat: Support TypeScript linting #54

Merged
merged 6 commits into from
Nov 9, 2020

Conversation

MichaelDeBoey
Copy link
Collaborator

@MichaelDeBoey MichaelDeBoey commented Apr 13, 2020


BREAKING CHANGE: Requires ESLint@^7.5.x
BREAKING CHANGE: (Optionally) requires TypeScript@^4.x

Closes #47

@MichaelDeBoey MichaelDeBoey marked this pull request as draft April 13, 2020 12:40
@MichaelDeBoey MichaelDeBoey changed the title Support type script feat: Support TypeScript linting Apr 13, 2020
@kentcdodds
Copy link
Owner

Sounds like eslint-find-rules doesn't support overrides (yet).

Also, we'll want to have a few TypeScript files in this project to run linting against to check the rules are configured properly.

@MichaelDeBoey
Copy link
Collaborator Author

@kentcdodds I'm not completely sure this is the right approach, what do you think?

@kentcdodds
Copy link
Owner

Commented 👍 Thanks.

@kentcdodds
Copy link
Owner

Oh, forgot about this PR. @MichaelDeBoey do you mind resolving conflicts? I think it's probably good to merge.

@MichaelDeBoey
Copy link
Collaborator Author

Totally forgot about it too

Will look into it this weekend to make sure I've updated all the rules 🙂

@MichaelDeBoey MichaelDeBoey force-pushed the support-TypeScript branch 5 times, most recently from 1757a54 to 046d717 Compare November 7, 2020 00:26
@MichaelDeBoey
Copy link
Collaborator Author

MichaelDeBoey commented Nov 7, 2020

@kentcdodds I've rebased my branch onto upstream/master and updated all dependencies to latest version (this is a breaking change) and overrode all rules with @typescript-eslint rules where possible.

I also turned off all rules that are handled by TS itself (according to plugin:@typescript-eslint/recommended)

Since eslint-find-rules doesn't support overrides (yet) (see eslint-find-rules), CI passes but the following Supported Rules aren't included (yet).

I suggest we definitely set all recommended rules to error, but I'm not sure what to do with the other ones.
Can you look into the table and let me know which ones need to be error, warn or off please and where to put these rules?
I'll make sure they're in the right place so this PR can be merged 🙂

best-practices.js Outdated Show resolved Hide resolved
es6/possible-errors.js Outdated Show resolved Hide resolved
es6/possible-errors.js Outdated Show resolved Hide resolved
@kentcdodds
Copy link
Owner

I'll spend time today going through every rule and deciding what I want it configured as. Wish me luck 😅

BREAKING CHANGE: Requires ESLint@^7.5.x
BREAKING CHANGE: (Optionally) requires TypeScript@^4.x
@kentcdodds
Copy link
Owner

Need to note this before I forget. We'll want to turn off react/prop-types for typescript I think.

@kentcdodds
Copy link
Owner

Alrighty, here's what I've got:

// 🙄 too opinionated
// 🤷‍♂️ no opinion
// 🤔 seems like a good idea
module.exports = {
  rules: {
    '@typescript-eslint/adjacent-overload-signatures': 'error', // 🤷‍♂️ (but it's recommended)
    '@typescript-eslint/array-type': 'off', // 🙄
    '@typescript-eslint/await-thenable': 'off', // await non-thenables can be handy
    '@typescript-eslint/ban-ts-comment': 'off', // 🙄
    '@typescript-eslint/ban-tslint-comment': 'error',
    '@typescript-eslint/ban-types': 'off', // not useful in a reusable config
    '@typescript-eslint/class-literal-property-style': 'off', // 🤷‍♂️
    '@typescript-eslint/consistent-indexed-object-style': 'off', // 🙄
    '@typescript-eslint/consistent-type-assertions': 'off', // 🤷‍♂️
    '@typescript-eslint/consistent-type-definitions': 'off', // 🙄
    '@typescript-eslint/consistent-type-imports': 'off', // I think I prefer typed imports, but you can't always use them
    '@typescript-eslint/explicit-function-return-type': 'off', // 🙄
    '@typescript-eslint/explicit-member-accessibility': 'off', // 🙄
    '@typescript-eslint/explicit-module-boundary-types': 'off', // 🙄
    '@typescript-eslint/member-ordering': 'off', // 🙄
    '@typescript-eslint/method-signature-style': 'off', // 🙄
    '@typescript-eslint/naming-convention': 'off', // 🙄
    '@typescript-eslint/no-base-to-string': 'warn', // 🤔 (if it works)
    '@typescript-eslint/no-confusing-non-null-assertion': 'off', // prettier reformats their "correct" example anyway 🤷‍♂️
    '@typescript-eslint/no-confusing-void-expression': 'off', // 🙄 (honestly, it's probably a good rule, but I do this all the time so...)
    '@typescript-eslint/no-dynamic-delete': 'off', // 🙄
    '@typescript-eslint/no-empty-interface': 'off', // 🙄
    '@typescript-eslint/no-explicit-any': 'off', // 🙄
    '@typescript-eslint/no-extra-non-null-assertion': 'error',
    '@typescript-eslint/no-extraneous-class': 'error', // stay away from classes when you can
    '@typescript-eslint/no-floating-promises': 'warn', // 🤔 not a bad rule, but can be annoying
    '@typescript-eslint/no-for-in-array': 'error',
    '@typescript-eslint/no-implicit-any-catch': 'warn',
    '@typescript-eslint/no-inferrable-types': 'off', // I personally prefer relying on inference where possible, but I don't want to lint for it.
    '@typescript-eslint/no-invalid-void-type': 'warn', // 🤔
    '@typescript-eslint/no-misused-new': 'error',
    '@typescript-eslint/no-misused-promises': 'warn', // 🤔
    '@typescript-eslint/no-namespace': 'error',
    '@typescript-eslint/no-non-null-asserted-optional-chain': 'error',
    '@typescript-eslint/no-non-null-assertion': 'error',
    '@typescript-eslint/no-parameter-properties': 'error', // yeah, I don't like this feature
    '@typescript-eslint/no-require-imports': 'off', // 🙄 sometimes you can't do it any other way
    '@typescript-eslint/no-this-alias': 'off', // 🙄
    '@typescript-eslint/no-type-alias': 'off', // 🙄
    '@typescript-eslint/no-unnecessary-boolean-literal-compare': 'warn',
    '@typescript-eslint/no-unnecessary-condition': 'error',
    '@typescript-eslint/no-unnecessary-qualifier': 'warn', // 🤷‍♂️ I'm not sure I understand this one
    '@typescript-eslint/no-unnecessary-type-arguments': 'off', // 🙄
    '@typescript-eslint/no-unnecessary-type-assertion': 'error',
    '@typescript-eslint/no-unnecessary-type-constraint': 'error',
    '@typescript-eslint/no-unsafe-assignment': 'warn', // 🤷‍♂️ seems like an ok idea, but I don't have enough experience with TS yet.
    '@typescript-eslint/no-unsafe-call': 'warn', // 🤷‍♂️ seems like an ok idea, but I don't have enough experience with TS yet.
    '@typescript-eslint/no-unsafe-member-access': 'warn', // 🤷‍♂️ seems like an ok idea, but I don't have enough experience with TS yet.
    '@typescript-eslint/no-unsafe-return': 'warn', // 🤷‍♂️ seems like an ok idea, but I don't have enough experience with TS yet.
    '@typescript-eslint/no-var-requires': 'off', // 🙄
    '@typescript-eslint/prefer-as-const': 'off', // 🙄
    '@typescript-eslint/prefer-enum-initializers': 'error', // makes total sense
    '@typescript-eslint/prefer-for-of': 'off', // 🙄 I prefer for of, but I don't want to lint for it
    '@typescript-eslint/prefer-function-type': 'off', // 🙄 (though I'm not sure I understand it)
    '@typescript-eslint/prefer-includes': 'error', // normally I'd 🙄 but includes is just so much better
    '@typescript-eslint/prefer-literal-enum-member': 'error', // 🤔
    '@typescript-eslint/prefer-namespace-keyword': 'error',
    '@typescript-eslint/prefer-nullish-coalescing': 'error',
    '@typescript-eslint/prefer-optional-chain': 'error',
    '@typescript-eslint/prefer-readonly': 'off', // 🙄
    '@typescript-eslint/prefer-readonly-parameter-types': 'off', // 🙄
    '@typescript-eslint/prefer-reduce-type-parameter': 'warn', // 🤔
    '@typescript-eslint/prefer-regexp-exec': 'off', // 🙄
    '@typescript-eslint/prefer-string-starts-ends-with': 'error',
    '@typescript-eslint/prefer-ts-expect-error': 'error',
    '@typescript-eslint/promise-function-async': 'off', // 🙄
    '@typescript-eslint/require-array-sort-compare': 'off', // 🙄
    '@typescript-eslint/restrict-plus-operands': 'error',
    '@typescript-eslint/restrict-template-expressions': 'off', // 🙄
    '@typescript-eslint/strict-boolean-expressions': 'off', // 🙄
    '@typescript-eslint/switch-exhaustiveness-check': 'error', // 🤔
    '@typescript-eslint/triple-slash-reference': [
      'error',
      {types: 'prefer-import'},
    ],
    '@typescript-eslint/typedef': 'off', // 🙄
    '@typescript-eslint/unbound-method': 'error',
    '@typescript-eslint/unified-signatures': 'warn', // 🤔
  },
}

I'm not sure where those rules should go. Gonna go grab lunch then I can sort them (or you're more than welcome to if you'd like).

@MichaelDeBoey
Copy link
Collaborator Author

MichaelDeBoey commented Nov 9, 2020

@kentcdodds
Copy link
Owner

Ah, nice. Let's disable that one.

@MichaelDeBoey MichaelDeBoey marked this pull request as ready for review November 9, 2020 22:31
Copy link
Owner

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stellar! Thank you so much! 👏

stylistic.js Outdated Show resolved Hide resolved
@kentcdodds kentcdodds merged commit 0cc600f into kentcdodds:master Nov 9, 2020
@MichaelDeBoey MichaelDeBoey deleted the support-TypeScript branch November 9, 2020 23:51
@github-actions
Copy link

github-actions bot commented Nov 9, 2020

🎉 This PR is included in version 17.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support TypeScript linting
2 participants