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

Jump to bottom

chore: enable typescript-eslint's recommended and stylistic configs internally #52948

Merged
merged 12 commits into from
Jul 31, 2023
Merged

chore: enable typescript-eslint's recommended and stylistic configs internally #52948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
82c36ad
chore: enable typescript-eslint's recommended and stylistic configs i…
JoshuaKGoldberg Jul 20, 2023
532a1aa
Whoops, forgot to check in package changes
JoshuaKGoldberg Jul 20, 2023
6e124d0
Reset pnpm-lock.yaml to canary, then reinstalled with right pnpm version
JoshuaKGoldberg Jul 20, 2023
e66cf37
Merge branch 'canary' into typescript-eslint-recommended-internal
styfle Jul 25, 2023
bf8552f
added a couple of suppressions
JoshuaKGoldberg Jul 26, 2023
7785336
merge canary
styfle Jul 31, 2023
e83fd0a
regenerate lockfile
styfle Jul 31, 2023
8ffc882
Merge branch 'canary' into typescript-eslint-recommended-internal
styfle Jul 31, 2023
2e767c5
prettier fix
styfle Jul 31, 2023
3210ea5
Merge branch 'canary' into typescript-eslint-recommended-internal
styfle Jul 31, 2023
1dd34cb
Recompile
styfle Jul 31, 2023
fd0a6b1
Merge branch 'canary' into typescript-eslint-recommended-internal
kodiakhq[bot] Jul 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
72 changes: 44 additions & 28 deletions .eslintrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,36 +43,51 @@
},
{ "files": ["**/__tests__/**"], "env": { "jest": true } },
{
"extends": [
"plugin:@typescript-eslint/recommended",
"plugin:@typescript-eslint/stylistic"
],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this doesn't enable type checked rules internally. I tried and they OOMd (typescript-eslint/typescript-eslint#1192). 😞

As a followup I'd love to try tinkering with the monorepo configuration (https://typescript-eslint.io/linting/typed-linting/monorepos) and/or using our new EXPERIMENTAL_useProjectService option (typescript-eslint/typescript-eslint#6754)

"files": ["**/*.ts", "**/*.tsx"],
"parser": "@typescript-eslint/parser",
"parserOptions": {
"ecmaVersion": 2018,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unnecessary configuration line. Per https://typescript-eslint.io/packages/parser#ecmaversion, 2018 is the default now.

"sourceType": "module",
"ecmaFeatures": {
"jsx": true
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unnecessary configuration line, I think (are there odd situations I didn't notice?). Per https://typescript-eslint.io/packages/parser#jsx, although it defaults to false, the TypeScript compiler will always parse .js, .mjs, .cjs, .jsx, and .tsx files with jsx: true.

Copy link
Member

Choose a reason for hiding this comment

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

What about .mts and .cts?

I think its best to keep jsx: true just in case. We don't want to introduce breaking changes in a patch release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about .mts and .cts?

From the same docs link: .ts, .mts, .cts files are always parsed as if this is false.

I'm curious: are you doing anything contrary to the default TypeScript behavior?

I think its best to keep jsx: true just in case. We don't want to introduce breaking changes in a patch release.

I'm under the impression this file only impacts Next.js repo development, not end-user development. Just to make sure we're on the same page: am I wrong about that? If so, how would this impact end users?

"warnOnUnsupportedTypeScriptVersion": false
},
"plugins": ["@typescript-eslint"],
"rules": {
// Already handled by TS
"no-dupe-class-members": "off",
"no-undef": "off",

// Add TypeScript specific rules (and turn off ESLint equivalents)
"@typescript-eslint/consistent-type-assertions": "warn",
"no-array-constructor": "off",
"@typescript-eslint/no-array-constructor": "warn",
// Todo: investigate, for each of these rules, whether we want them.
"@typescript-eslint/array-type": "off",
"@typescript-eslint/ban-ts-comment": "off",
"@typescript-eslint/ban-tslint-comment": "off",
"@typescript-eslint/ban-types": "off",
"@typescript-eslint/class-literal-property-style": "off",
"@typescript-eslint/consistent-generic-constructors": "off",
"@typescript-eslint/consistent-indexed-object-style": "off",
"@typescript-eslint/consistent-type-definitions": "off",
"@typescript-eslint/no-empty-function": "off",
"@typescript-eslint/no-namespace": "off",
"no-use-before-define": "off",
"@typescript-eslint/no-use-before-define": [
"warn",
"@typescript-eslint/no-shadow": "off",
"@typescript-eslint/no-empty-interface": "off",
"@typescript-eslint/no-explicit-any": "off",
"@typescript-eslint/no-inferrable-types": "off",
"@typescript-eslint/no-var-requires": "off",
"@typescript-eslint/prefer-for-of": "off",
"@typescript-eslint/prefer-function-type": "off",
"@typescript-eslint/no-this-alias": "off",
"@typescript-eslint/triple-slash-reference": "off",
"no-var": "off",
"prefer-const": "off",
"prefer-rest-params": "off",
"prefer-spread": "off",

// These off- or differently-configured rules work well for us.
"no-unused-expressions": "off",
"@typescript-eslint/no-unused-expressions": [
"error",
{
"functions": true,
"classes": true,
"variables": true,
"enums": true,
"typedefs": true
"allowShortCircuit": true,
"allowTernary": true,
"allowTaggedTemplates": true
}
],
"no-unused-vars": "off",
Expand All @@ -83,19 +98,20 @@
"ignoreRestSiblings": true
}
],
"no-unused-expressions": "off",
"@typescript-eslint/no-unused-expressions": [
"error",
"no-use-before-define": "off",
"@typescript-eslint/no-use-before-define": [
"warn",
{
"allowShortCircuit": true,
"allowTernary": true,
"allowTaggedTemplates": true
"functions": true,
"classes": true,
"variables": true,
"enums": true,
"typedefs": true
}
],
"no-useless-constructor": "off",
"@typescript-eslint/no-useless-constructor": "warn",
"@typescript-eslint/prefer-literal-enum-member": "error",
"@typescript-eslint/prefer-namespace-keyword": "error"
"@typescript-eslint/prefer-literal-enum-member": "error"
},
"overrides": [
{
Expand Down
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 @@ -102,8 +102,8 @@
"@types/sharp": "0.29.3",
"@types/string-hash": "1.1.1",
"@types/trusted-types": "2.0.3",
"@typescript-eslint/eslint-plugin": "4.29.1",
"@typescript-eslint/parser": "4.29.1",
"@typescript-eslint/eslint-plugin": "6.1.0",
"@typescript-eslint/parser": "6.1.0",
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
"@vercel/fetch": "6.1.1",
"@vercel/og": "0.5.9",
"@zeit/next-typescript": "1.1.2-canary.0",
Expand Down
2 changes: 1 addition & 1 deletion packages/eslint-plugin-next/src/utils/url.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ export function getUrlFromPagesDirectories(
})
}

export function execOnce<TArgs extends any[], TResult extends unknown>(
export function execOnce<TArgs extends any[], TResult>(
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://typescript-eslint.io/rules/no-unnecessary-type-constraint: extends unknown doesn't do anything for a generic type parameter. Dead (type system) code.

fn: (...args: TArgs) => TResult
): (...args: TArgs) => TResult {
let used = false
Expand Down
2 changes: 2 additions & 0 deletions packages/next-env/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ export function processEnv(
typeof parsed[key] === 'undefined' &&
typeof origEnv[key] === 'undefined'
) {
// We're being imprecise in the type system - assume parsed[key] can be undefined
// eslint-disable-next-line @typescript-eslint/no-non-null-asserted-optional-chain
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See https://twitter.com/mattpocockuk/status/1681267079977000961 from our friend @mattpocock about why I'm not going to try to get this working perfectly 😄

Copy link
Member

Choose a reason for hiding this comment

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

How about changing Object.keys() to Object.entries()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That might work! I've learned the hard way to not try to change runtime behavior in static analysis oriented PRs like this one. If you really want me to make this change I can - but would suggest moving it to a separate PR as a separate cleanup item.

parsed[key] = result.parsed?.[key]!
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/next/src/build/webpack/plugins/middleware-plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ function getCodeAnalyzer(params: {
buildInfo.importLocByPath = new Map()
}

const importedModule = node.source.value?.toString()!
const importedModule = node.source.value?.toString()
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://typescript-eslint.io/rules/no-non-null-asserted-optional-chain: putting a ! ("this must exist") after a ?. ("this might not exist") is almost certainly a sign of something being wrong in the type system understanding of the code. Here, the ! didn't do anything because node is type any.

buildInfo.importLocByPath.set(importedModule, {
sourcePosition: {
...node.loc.start,
Expand Down
2 changes: 2 additions & 0 deletions packages/next/src/client/webpack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
// eslint-disable-next-line @typescript-eslint/no-unused-vars
declare const __webpack_require__: any
// eslint-disable-next-line @typescript-eslint/no-unused-vars
declare let __webpack_public_path__: string

const addChunkSuffix =
Expand Down
8 changes: 4 additions & 4 deletions packages/next/src/compiled/micromatch/index.js
View file
Open in desktop

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions packages/next/src/lib/metadata/types/metadata-interface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,7 @@ type SitemapFile = Array<{

type ResolvingMetadata = Promise<ResolvedMetadata>
declare namespace MetadataRoute {
// eslint-disable-next-line @typescript-eslint/no-shadow
export type Robots = RobotsFile
export type Sitemap = SitemapFile
export type Manifest = ManifestFile
Expand Down
1 change: 1 addition & 0 deletions packages/next/src/shared/lib/fnv1a.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// source: https://github.com/sindresorhus/fnv1a
// FNV_PRIMES and FNV_OFFSETS from
// http://www.isthe.com/chongo/tech/comp/fnv/index.html#FNV-param
/* eslint-disable @typescript-eslint/no-loss-of-precision */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this is copy & pasted from an external source that deals with numbers, I don't think we really needed to flag for correct behavior around numeric precision. 😄

For reference, https://typescript-eslint.io/rules/no-loss-of-precision/ (and also typescript-eslint/typescript-eslint#7277 noting that the typescript-eslint extension might not be necessary anymore)


const FNV_PRIMES = {
32: BigInt(16_777_619),
Expand Down