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.

Sign up for GitHub

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
Merged

feat: typescript-eslint@v6 #1476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5a24c6c
feat: typescript-eslint@v6
JoshuaKGoldberg Jun 19, 2023
f4efe55
Corrected versions with resolutions
JoshuaKGoldberg Jun 19, 2023
b9f5c96
Switch _eslintrc.cjs back to const config
JoshuaKGoldberg Jun 19, 2023
0cc41a3
Undid no-confusing-void-expression
JoshuaKGoldberg Jun 19, 2023
9772933
Merge branch 'next'
JoshuaKGoldberg Jun 19, 2023
47bfca9
typescript-eslint@v6 stable 🥳
JoshuaKGoldberg Jul 11, 2023
81b57af
Merge branch 'next'
JoshuaKGoldberg Jul 11, 2023
8f182d0
Finished migrating to stable release
JoshuaKGoldberg Jul 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/modern-parents-float.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"create-t3-app": minor
---

Upgraded typescript-eslint to v6, with reworked ESLint configurations
28 changes: 15 additions & 13 deletions .eslintrc.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ const config = {
parser: "@typescript-eslint/parser",
plugins: ["isaacscript", "import"],
extends: [
"plugin:@typescript-eslint/recommended",
"plugin:@typescript-eslint/recommended-requiring-type-checking",
"plugin:@typescript-eslint/recommended-type-checked",
"plugin:@typescript-eslint/stylistic-type-checked",
"plugin:prettier/recommended",
],
parserOptions: {
Expand All @@ -19,8 +19,16 @@ const config = {
"./www/tsconfig.json",
],
},
overrides: [
// Template files don't have reliable type information
{
files: ["./cli/template/**/*.{ts,tsx}"],
extends: ["plugin:@typescript-eslint/disable-type-checked"],
},
],
rules: {
"@typescript-eslint/restrict-template-expressions": "off",
// These off/not-configured-the-way-we-want lint rules we like & opt into
"@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 •
edited
Loading

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.

Copy link

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

"@typescript-eslint/no-unused-vars": [
"error",
{ argsIgnorePattern: "^_", destructuredArrayIgnorePattern: "^_" },
Expand All @@ -31,19 +39,13 @@ const config = {
],
"import/consistent-type-specifier-style": ["error", "prefer-inline"],

// These rules are only disabled because we hit a bug in linting.
// See https://github.com/t3-oss/create-t3-app/pull/1036#discussion_r1060505136
// If you still see the bug once TypeScript@5 is used, please let typescript-eslint know!
"@typescript-eslint/no-unsafe-argument": "off",
"@typescript-eslint/no-unsafe-assignment": "off",
"@typescript-eslint/no-unsafe-call": "off",
"@typescript-eslint/no-unsafe-member-access": "off",
"@typescript-eslint/no-unsafe-return": "off",
"@typescript-eslint/no-unnecessary-type-assertion": "off",

// For educational purposes we format our comments/jsdoc nicely
"isaacscript/complete-sentences-jsdoc": "warn",
"isaacscript/format-jsdoc-comments": "warn",

// These lint rules don't make sense for us but are enabled in the preset configs
"@typescript-eslint/no-confusing-void-expression": "off",
"@typescript-eslint/restrict-template-expressions": "off",
},
};

Expand Down
1 change: 1 addition & 0 deletions cli/.eslintignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
dist
6 changes: 3 additions & 3 deletions cli/src/helpers/installDependencies.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 😅

projectDir: string;
};
}

/*eslint-disable @typescript-eslint/no-floating-promises*/
const runInstallCommand = async (
Expand Down Expand Up @@ -76,7 +76,7 @@ export const installDependencies = async ({ projectDir }: Options) => {

// If the spinner was used to show the progress, use succeed method on it
// If not, use the succeed on a new spinner
(installSpinner || ora()).succeed(
(installSpinner ?? ora()).succeed(
chalk.green("Successfully installed dependencies!\n")
);
};
4 changes: 2 additions & 2 deletions cli/src/helpers/installPackages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ import {
} from "~/installers/index.js";
import { logger } from "~/utils/logger.js";

type InstallPackagesOptions = {
type InstallPackagesOptions = InstallerOptions & {
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 •
edited
Loading

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.

// This runs the installer for all the packages that the user has selected
export const installPackages = (options: InstallPackagesOptions) => {
const { packages } = options;
Expand Down
2 changes: 1 addition & 1 deletion cli/src/helpers/logNextSteps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export const logNextSteps = ({
projectName = DEFAULT_APP_NAME,
packages,
noInstall,
}: Pick<InstallerOptions, "projectName" | "packages" | "noInstall">) => {
}: Pick<InstallerOptions, "noInstall" | "packages" | "projectName">) => {
const pkgManager = getUserPkgManager();

logger.info("Next steps:");
Expand Down
2 changes: 1 addition & 1 deletion cli/src/helpers/selectBoilerplate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { PKG_ROOT } from "~/consts.js";
import { type InstallerOptions } from "~/installers/index.js";

type SelectBoilerplateProps = Required<
Pick<InstallerOptions, "projectDir" | "packages">
Pick<InstallerOptions, "packages" | "projectDir">
>;
// This generates the _app.tsx file that is used to render the app
export const selectAppFile = ({
Expand Down
4 changes: 2 additions & 2 deletions cli/src/utils/renderVersionWarning.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ export const renderVersionWarning = (npmVersion: string) => {
* directory of this source tree.
* https://github.com/facebook/create-react-app/blob/main/packages/create-react-app/LICENSE
*/
type DistTagsBody = {
interface DistTagsBody {
latest: string;
};
}

function checkForLatestVersion(): Promise<string> {
return new Promise((resolve, reject) => {
Expand Down
22 changes: 6 additions & 16 deletions cli/template/base/_eslintrc.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,15 @@
// eslint-disable-next-line @typescript-eslint/no-var-requires
const path = require("path");

/** @type {import("eslint").Linter.Config} */
const config = {
overrides: [
{
extends: [
"plugin:@typescript-eslint/recommended-requiring-type-checking",
],
files: ["*.ts", "*.tsx"],
parserOptions: {
project: path.join(__dirname, "tsconfig.json"),
},
},
],
parser: "@typescript-eslint/parser",
parserOptions: {
project: path.join(__dirname, "tsconfig.json"),
project: true,
},
plugins: ["@typescript-eslint"],
extends: ["next/core-web-vitals", "plugin:@typescript-eslint/recommended"],
extends: [
"next/core-web-vitals",
"plugin:@typescript-eslint/recommended-type-checked",
"plugin:@typescript-eslint/stylistic-type-checked",
],
rules: {
"@typescript-eslint/consistent-type-imports": [
"warn",
Expand Down
4 changes: 2 additions & 2 deletions cli/template/base/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@
"@types/node": "^18.16.0",
"@types/react": "^18.2.6",
"@types/react-dom": "^18.2.4",
"@typescript-eslint/eslint-plugin": "^5.59.6",
"@typescript-eslint/parser": "^5.59.6",
"@typescript-eslint/eslint-plugin": "6.0.0",
"@typescript-eslint/parser": "6.0.0",
"eslint": "^8.40.0",
"eslint-config-next": "^13.4.2",
"typescript": "^5.0.4"
Expand Down
6 changes: 3 additions & 3 deletions cli/template/extras/src/server/api/trpc/with-auth-prisma.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ import { prisma } from "~/server/db";
* These allow you to access things when processing a request, like the database, the session, etc.
*/

type CreateContextOptions = {
interface CreateContextOptions {
session: Session | null;
};
}

/**
* This helper generates the "internals" for a tRPC context. If you need to use it, you can export
Expand Down Expand Up @@ -108,7 +108,7 @@ export const publicProcedure = t.procedure;

/** Reusable middleware that enforces users are logged in before running the procedure. */
const enforceUserIsAuthed = t.middleware(({ ctx, next }) => {
if (!ctx.session || !ctx.session.user) {
if (!ctx.session?.user) {
throw new TRPCError({ code: "UNAUTHORIZED" });
}
return next({
Expand Down
6 changes: 3 additions & 3 deletions cli/template/extras/src/server/api/trpc/with-auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,9 @@ import { getServerAuthSession } from "~/server/auth";
* These allow you to access things when processing a request, like the database, the session, etc.
*/

type CreateContextOptions = {
interface CreateContextOptions {
session: Session | null;
};
}

/**
* This helper generates the "internals" for a tRPC context. If you need to use it, you can export
Expand Down Expand Up @@ -106,7 +106,7 @@ export const publicProcedure = t.procedure;

/** Reusable middleware that enforces users are logged in before running the procedure. */
const enforceUserIsAuthed = t.middleware(({ ctx, next }) => {
if (!ctx.session || !ctx.session.user) {
if (!ctx.session?.user) {
throw new TRPCError({ code: "UNAUTHORIZED" });
}
return next({
Expand Down
4 changes: 2 additions & 2 deletions cli/template/extras/src/server/auth/base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ import { env } from "~/env.mjs";
*/
declare module "next-auth" {
interface Session extends DefaultSession {
user: {
user: DefaultSession["user"] & {
id: string;
// ...other properties
// role: UserRole;
} & DefaultSession["user"];
};
}

// interface User {
Expand Down
4 changes: 2 additions & 2 deletions cli/template/extras/src/server/auth/with-prisma.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ import { prisma } from "~/server/db";
*/
declare module "next-auth" {
interface Session extends DefaultSession {
user: {
user: DefaultSession["user"] & {
id: string;
// ...other properties
// role: UserRole;
} & DefaultSession["user"];
};
}

// interface User {
Expand Down
1 change: 1 addition & 0 deletions cli/tsconfig.eslint.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.

"include": ["src", "template", "tsup.config.ts"]
}
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@
"@types/eslint": "^8.37.0",
"@types/node": "^18.16.0",
"@types/prettier": "^2.7.2",
"@typescript-eslint/eslint-plugin": "^5.59.6",
"@typescript-eslint/parser": "^5.59.6",
"@typescript-eslint/eslint-plugin": "6.0.0",
"@typescript-eslint/parser": "6.0.0",
"eslint": "^8.40.0",
"eslint-config-prettier": "^8.8.0",
"eslint-config-turbo": "^0.0.9",
Expand Down
Loading