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

Updated typescript-eslint to v6 #54693

Merged
merged 13 commits into from
Jul 14, 2023

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Jun 18, 2023

Applies the following strategy for updating the linter:

  1. Bumping the version to the latest beta (technically rc, but there's no meaningful difference here)
  2. Extended from our two recommended starting non-type-checked configs: recommended and stylistic
  3. Removed any existing rule configurations that are redundant when extending those configs
  4. Disabled any rules with existing violations that seemed intentional, under a "not applicable here" comment
  5. Disabled any rules with existing violations that I wasn't sure the intentionality behind, under a "Todo: do we want these?" comment
  6. Manually fixed up remaining lint complaints

Explanations for the changes are inline. I realize the mild hubris on my end of posting lint recommendations on the TypeScript codebase itself - seeking input as to whether we're going the wrong way on any of them. 😄

For the Todo rules, I'm interested in your opinions on whether you'd want each of them or not in the TypeScript codebase.

Fixes #54692

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jun 18, 2023
@@ -5732,7 +5732,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return rightMeaning === SymbolFlags.Value ? SymbolFlags.Value : SymbolFlags.Namespace;
}

function getAccessibleSymbolChain(symbol: Symbol | undefined, enclosingDeclaration: Node | undefined, meaning: SymbolFlags, useOnlyExternalAliasing: boolean, visitedSymbolTablesMap: Map<SymbolId, SymbolTable[]> = new Map()): Symbol[] | undefined {
function getAccessibleSymbolChain(symbol: Symbol | undefined, enclosingDeclaration: Node | undefined, meaning: SymbolFlags, useOnlyExternalAliasing: boolean, visitedSymbolTablesMap = new Map<SymbolId, SymbolTable[]>()): Symbol[] | undefined {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@typescript-eslint/consistent-generic-constructors: "The generic type arguments should be specified as part of the constructor type arguments."

This comes from the stylistic config - it's a little more succinct to put the <...> type parameters after the new Map. The rule is auto-fixable.

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review June 22, 2023 15:06
@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 22, 2023
@sandersn sandersn self-requested a review June 26, 2023 22:09
@sandersn
Copy link
Member

mild hubris on my end of posting lint recommendations on the TypeScript codebase itself

On the contrary; we really really don't want our codebase to be the basis for good Typescript style. eslint is absolutely the right provider of those defaults.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Comments on the TODO-commented rules.

👍🏼 means that I like the value given in the PR; I commented on the others.

src/compiler/checker.ts Outdated Show resolved Hide resolved
.eslintrc.json Show resolved Hide resolved
.eslintrc.json Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
"@typescript-eslint/ban-types": "off",
"@typescript-eslint/no-empty-function": "off",
"@typescript-eslint/no-empty-interface": "off",
"@typescript-eslint/no-unused-vars": "off",
Copy link
Member

Choose a reason for hiding this comment

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

Is this off because the tsconfig has the equivalent Typescript setting on? If not, what kind and how many unused variables do we have?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's this; honestly I would much prefer if we didn't use TS for this at all and instead made it a lint. So annoying to have to ensure my variables are all used just to run tests or something. At least debug skips typecheck.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only 18 failures at the moment. Not bad!

However, typescript-eslint/typescript-eslint#5017 -> gajus/eslint-plugin-jsdoc#858 is impacting the codebase here. Probably best to set up the workarounds in a separate PR IMO.

"@typescript-eslint/sort-type-constituents": "off",

// Pending https://github.com/typescript-eslint/typescript-eslint/issues/4820
"@typescript-eslint/prefer-optional-chain": "off",
Copy link
Member

Choose a reason for hiding this comment

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

Is this off because it causes lots of churn? If so, I'd like to see it turned on, but maybe in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, and also because the rule has more false positives right now than I'm comfortable with - especially in a codebase like TypeScript's that does a lot of funky stuff with scoping.

Copy link
Member

Choose a reason for hiding this comment

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

I'm honestly shocked we aren't getting lints from prefer-nullish-coalescing too... I was restructuring the lint config of hereby and hit some false positives there too on code like:

declare const a: string | undefined
declare const b: string

const foo = a || b;

Where the actual intent is that the empty string goes away. But maybe that's not a false positive.

.eslintrc.json Outdated
"@typescript-eslint/no-empty-function": "off",
"@typescript-eslint/no-empty-interface": "off",
"@typescript-eslint/no-unused-vars": "off",
"@typescript-eslint/sort-type-constituents": "off",
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼

@sandersn
Copy link
Member

@JoshuaKGoldberg I read your replies. It sounds to me like this PR is in a good state, and then needs 3 or 4 follow ups to turn on even more lint rules (and also turn off the tsconfig setting for checking unused variables). Does that sound about right to you?

@JoshuaKGoldberg
Copy link
Contributor Author

@sandersn that's correct!

@sandersn
Copy link
Member

sandersn commented Jul 5, 2023

I forgot to ask: do you want to merge this with v6 beta or wait for the full release?

I'll ask the rest of the team to see if anybody there cares one way or the other as well.
Edit: Yep, we're OK with the beta if you are.

@JoshuaKGoldberg
Copy link
Contributor Author

Beta works for me! We've scheduled the v6 announcement blog post to go out on Monday (🤞, knock on wood, etc.). It's no trouble at all. And, bonus, anything that bumps up my TypeScript PR count makes me happy 😄

@sandersn
Copy link
Member

sandersn commented Jul 6, 2023

Looks like there's some weirdness with failing tests (jsdocThisType), so probably needs a merge from main.

@jakebailey
Copy link
Member

If we're going to merge this, probably it's worth merging in proximity to #54835 to reduce the speedbump of the dep change?

@jakebailey
Copy link
Member

Also, this PR is still pinned back to an old alpha, does that need to be bumped? Or are we just waiting for a "more real" release?

@JoshuaKGoldberg
Copy link
Contributor Author

JoshuaKGoldberg commented Jul 6, 2023

The alphas aren't changing much release-to-release at this point. I bumped to the latest in 0fde8d4 and the only relevant change was a couple rule changes in the used configs (typescript-eslint/typescript-eslint#7157).

Edit: so, actually answering: no, this doesn't need to bump to new alphas.

@JoshuaKGoldberg
Copy link
Contributor Author

Btw, I just updated to typescript-eslint@v6.0.0 stable. 🥳

@jakebailey jakebailey changed the title Updated typescript-eslint to v6 beta Updated typescript-eslint to v6 Jul 11, 2023
@jakebailey
Copy link
Member

Updated the title to reflect that.

package.json Outdated Show resolved Hide resolved
.eslintrc.json Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

This seems good to me now, though given how many lints we disable, I half wonder if we should just disable the : Map lint then send them all as follow-ups.

Here's a diff for the eslint rules that can be dropped as they are part of the eslint recommended set:

diff --git a/.eslintrc.json b/.eslintrc.json
index 56d271a3f4..69ec74d564 100644
--- a/.eslintrc.json
+++ b/.eslintrc.json
@@ -109,18 +109,18 @@
         "no-null/no-null": "error",
 
         // eslint
-        "constructor-super": "error",
+        // "constructor-super": "error",
         "curly": ["error", "multi-line"],
         "dot-notation": "error",
         "eqeqeq": "error",
         "linebreak-style": ["error", "windows"],
         "new-parens": "error",
         "no-caller": "error",
-        "no-duplicate-case": "error",
-        "no-empty": "error",
+        // "no-duplicate-case": "error",
+        // "no-empty": "error",
         "no-eval": "error",
         "no-extra-bind": "error",
-        "no-fallthrough": "error",
+        // "no-fallthrough": "error",
         "no-new-func": "error",
         "no-new-wrappers": "error",
         "no-return-await": "error",
@@ -132,24 +132,24 @@
             { "name": "setImmediate" },
             { "name": "clearImmediate" }
         ],
-        "no-sparse-arrays": "error",
+        // "no-sparse-arrays": "error",
         "no-template-curly-in-string": "error",
         "no-throw-literal": "error",
         "no-trailing-spaces": "error",
         "no-undef-init": "error",
-        "no-unsafe-finally": "error",
-        "no-unused-labels": "error",
+        // "no-unsafe-finally": "error",
+        // "no-unused-labels": "error",
         "no-var": "error",
         "object-shorthand": "error",
         "prefer-const": "error",
         "prefer-object-spread": "error",
         "quote-props": ["error", "consistent-as-needed"],
         "space-in-parens": "error",
-        "unicode-bom": ["error", "never"],
-        "use-isnan": "error",
-        "no-prototype-builtins": "error",
-        "no-self-assign": "error",
-        "no-dupe-else-if": "error"
+        "unicode-bom": ["error", "never"]
+        // "use-isnan": "error",
+        // "no-prototype-builtins": "error",
+        // "no-self-assign": "error",
+        // "no-dupe-else-if": "error"
     },
     "overrides": [
         // By default, the ESLint CLI only looks at .js files. But, it will also look at

I also want to (not in this PR) reorganize this eslint config to distinguish which rules are effectively formatting rules and which are not; I feel like the current ordering by plugin is not really very helpful now that we are using extends, and I already know which rules are going to go away when we dprint.

@jakebailey
Copy link
Member

@JoshuaKGoldberg You may have missed it, but the code box above has a scrollbar and there are 8 other ones to also remove.

@jakebailey jakebailey merged commit 0ae8ca1 into microsoft:main Jul 14, 2023
@JoshuaKGoldberg JoshuaKGoldberg deleted the typescript-eslint-v6 branch July 15, 2023 18:32
@JoshuaKGoldberg
Copy link
Contributor Author

Ah, great - thanks for finishing this up!

...I did miss the scrollbar 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Tooling: Update typescript-eslint to v6
5 participants