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: typescript-eslint@v6 #1476

Merged
merged 8 commits into from
Jul 12, 2023

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Jun 19, 2023

Closes #1474 (ref: typescript-eslint/typescript-eslint#6760)

✅ Checklist

  • I have followed every step in the contributing guide (updated 2022-10-06).
  • The PR title follows the convention we established conventional-commit
  • I performed a functional test on my final commit

Changelog

Upgraded typescript-eslint to v6, with reworked ESLint configurations.

You can read https://typescript-eslint.io/blog/announcing-typescript-eslint-v6-beta#user-facing-breaking-changes for the rationale behind the config changes. Essentially, the new recommended starter configs are:

  • "plugin:@typescript-eslint/recommended-type-checked"
  • "plugin:@typescript-eslint/stylistic-type-checked"

I've commented any changes I suspect you might disagree with inline, for discussion. Would love to know what you all think!

💯

@changeset-bot
Copy link

changeset-bot bot commented Jun 19, 2023

🦋 Changeset detected

Latest commit: 8f182d0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
create-t3-app Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Jun 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
create-t3-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 11, 2023 10:37pm

@vercel
Copy link

vercel bot commented Jun 19, 2023

@JoshuaKGoldberg is attempting to deploy a commit to the t3-oss Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added 📌 area: cli Relates to the CLI 📌 area: t3-app Relates to the generated T3 App 📚 documentation Improvements or additions to documentation labels Jun 19, 2023
rules: {
"@typescript-eslint/no-explicit-any": "error",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We right now have no-explicit-any moved into the strict config for v6. But I understand you were expecting it to be enabled. I'm now wondering if we should keep it at recommended?

For context, strict is something we recommend for more confident TypeScript users. https://v6--typescript-eslint.netlify.app/linting/configs#strict

Copy link
Member

Choose a reason for hiding this comment

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

so what's the main difference in strictness between

strict-type-checked and the old recommended-requiring-type-checking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The strict & strict-type-checked rulesets are a lot more opinionated. We phrase them as "Additional strict rules that can also catch bugs but are more opinionated than recommended rules.". In v6 they're supersets of the respective recommended & recommended-type-checked configs.

https://v6--typescript-eslint.netlify.app/rules/?supported-rules=xrecommended-strict shows the rules that are in strict* and not recommended*.

Copy link
Member

@c-ehrlich c-ehrlich Jun 19, 2023

Choose a reason for hiding this comment

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

after reading through the list, i would definitely want about 2/3 of the strict rules and am neutral-but-could-be-pushed-positive on the remaining 1/3.

for teams that are less experienced at typescript, i think autofixing where possible + good documentation for the errors could move quite a few of the rules into "reasonable for most teams" territory.

what is the motivation for moving no-explicit-any to the strict rules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC it was that a lot of projects end up needing to use any (or at least their developers think they do), and we're trying to not be in people's way as much with the recommended rulesets. In a sense this is us capitulating to how new & intermediate TypeScript developers write code (the occasional any, not always doing type safe things). Capitulating means less friction in turning on the recommended rulesets, at the cost of being less useful out-of-the-box.

Choose a reason for hiding this comment

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

Do you think educating those users to try to use unknown instead could help adoption?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, definitely. A lot of these lint rules are better from user education - and can themselves act as user education!

Which reminds me, https://typescript-eslint.io/rules/no-explicit-any could do a bit better in that regard. Filed typescript-eslint/typescript-eslint#7136. Thanks! 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just following up, I finally got around to documenting this: typescript-eslint/typescript-eslint#8370

@github-actions
Copy link
Contributor

github-actions bot commented Jun 19, 2023

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 95
🟢 Accessibility 100
🟢 Best practices 100
🟢 SEO 100
🔴 PWA 30

Lighthouse ran on https://create-t3-app-git-fork-joshuakgoldberg-typescript-f05f1a-t3-oss.vercel.app/

@@ -7,9 +7,9 @@ import {
} from "~/utils/getUserPkgManager.js";
import { logger } from "~/utils/logger.js";

type Options = {
interface Options {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://v6--typescript-eslint.netlify.app/rules/consistent-type-definitions is the rule to enforce which of interface or type you prefer to use consistently. In v6 right now it's moved from strict to stylistic. How does that make you feel?

For reference, a couple of full text regex searches on the next branch:

  • interface .+ \{: 44 results in 40 files
  • type .+ = \{: 14 results in 12 files

Copy link
Member

Choose a reason for hiding this comment

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

this is one of the few that i don't feel strongly about.

my personal preference is "type by default, interface only if necessary (ie need to extend)" but judging by these stats that's not actually what we're doing currently.

what's the eslint team's opinion on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We in typescript-eslint1 have interface over type as the default in our recommended rulesets, but I don't believe any of us feel extremely strongly about it. I personally don't. IMO the most important thing for a codebase is to standardize on one to reduce confusion.

I personally have leaned towards interfaces because they've historically had better error messages in edge cases than type. Though over the last couple of years, TypeScript has gotten a lot better at using names for types. So that's not as powerful a motivator as it used to be...

Footnotes

  1. Sorry to nitpick - we're a separate team from ESLint core and don't want to accidentally give the impression we're speaking for them 😅

Copy link

@nickserv nickserv Jun 23, 2023

Choose a reason for hiding this comment

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

I recommend using type for literals and primitives (also unions and intersections of them), and interface for all other types. The Performance page in the official TypeScript wiki recommends interface for object types because of performance options and the ability to augment types.

Copy link
Member

Choose a reason for hiding this comment

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

one big argument in favour of "type by default" is that you can't accidentally merge with globals, see https://tsplay.dev/wjanvW

and i guess it also lets you know when you read a signature that what you're looking at is what you'll get as no merging can happen anywhere down the line.

but i agree by far the most important thing is picking a set of rules and sticking with them. i'd like create-t3-app to use whatever can reasonably be called the "default" behaviour for this sort of stuff unless there's a very good reason not to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be clear - would you like me to disable the rule and revert its changes? I'm happy to, just not sure from the discussion 😅

packages: PkgInstallerMap;
} & InstallerOptions;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://v6--typescript-eslint.netlify.app/rules/sort-type-constituents - is newly in the stylistic config. While it does have an autofixer, I do worry that it's too nitpicky and will annoy people. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

i think i like it, makes it more like interface where the things you extend comes first

Copy link
Contributor Author

@JoshuaKGoldberg JoshuaKGoldberg Jun 19, 2023

Choose a reason for hiding this comment

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

Great - if we end up removing it from the config, I can manually re-enable it in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

i like this a lot as well. if i'm reading an interface, i don't want to miss the fact that it extends something.

@@ -1,3 +1,4 @@
{
"extends": "./tsconfig.json",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, the type checked rules don't have the right compiler options: most notably strict: true.

package.json Outdated
"@typescript-eslint/parser": "6.0.0-alpha.158",
"@typescript-eslint/scope-manager": "6.0.0-alpha.158",
"@typescript-eslint/types": "6.0.0-alpha.158",
"@typescript-eslint/visitor-keys": "6.0.0-alpha.158"
Copy link
Contributor Author

@JoshuaKGoldberg JoshuaKGoldberg Jun 19, 2023

Choose a reason for hiding this comment

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

Re versioning: I'm guessing you won't want to merge this PR until we're out of alpha/beta/rc and in 6.0.0 stable. Noting here explicitly because otherwise I think this PR is ready for review. 🙂

Copy link
Member

Choose a reason for hiding this comment

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

👍🏼

why are the resolutions needed though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some nested dependencies still rely on 5.x. If you remove them locally & re-run pnpm i then the pnpm-lock.yaml gets a bit bigger. Example:

  /astro-eslint-parser@0.14.0:
    resolution: {integrity: sha512-3F8l1h7+5MNxzDg1cSQxEloalG7fj64K6vOERChUVG7RLnAzSoafADnPQlU8DpMM3WRNfRHSC4NwUCORk/aPrA==}
    engines: {node: ^14.18.0 || >=16.0.0}
    dependencies:
      '@astrojs/compiler': 1.4.1
      '@typescript-eslint/scope-manager': 5.59.11
      '@typescript-eslint/types': 5.59.11

Copy link
Member

Choose a reason for hiding this comment

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

i see - is this just needed when trying the beta and something that can be removed later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be yeah. This PR was useful for me in seeing who in the community we need to ask to add || 6 support! 😄

@juliusmarminge
Copy link
Member

Do you also have updates on the new config stuff in eslint? https://eslint.org/docs/latest/use/configure/configuration-files-new

I'm interested in trying that out but last time i tried it was a bit broken

@JoshuaKGoldberg
Copy link
Contributor Author

Do you also have updates on the new config stuff in eslint? eslint.org/docs/latest/use/configure/configuration-files-new

Not yet 😞 but once it's past stage 3 (eslint/eslint#13481) I'd be happy to!

@JoshuaKGoldberg
Copy link
Contributor Author

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

cli/template/base/package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@juliusmarminge juliusmarminge enabled auto-merge July 12, 2023 12:12
@juliusmarminge
Copy link
Member

Thank you @JoshuaKGoldberg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📌 area: cli Relates to the CLI 📌 area: t3-app Relates to the generated T3 App 📚 documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Use typescript-eslint@v6's reworked configs
4 participants