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

chore: Upgrade ESLint to v9 #24273

Merged
merged 5 commits into from
Jun 4, 2024
Merged

chore: Upgrade ESLint to v9 #24273

merged 5 commits into from
Jun 4, 2024

Conversation

nzakas
Copy link
Contributor

@nzakas nzakas commented May 23, 2024

This is an attempt to upgrade the repo to ESLint v9.

I've verified that the new config file works and ESLint runs. However, there are some outstanding ESLint errors that need review from someone familiar with the codebase. I believe these are valid, but they didn't show up before the migration, so it's possible you may want to ignore them.

C:\Users\nzaka\projects\third-party\prisma\helpers\blaze\omit.ts      
  11:45  error  Unexpected constant truthiness on the left-hand side of a `&&` expression  no-constant-binary-expression
  17:45  error  Unexpected constant truthiness on the left-hand side of a `&&` expression  no-constant-binary-expression

C:\Users\nzaka\projects\third-party\prisma\helpers\blaze\pick.ts      
  11:45  error  Unexpected constant truthiness on the left-hand side of a `&&` expression  no-constant-binary-expression
  17:45  error  Unexpected constant truthiness on the left-hand side of a `&&` expression  no-constant-binary-expression

C:\Users\nzaka\projects\third-party\prisma\packages\nextjs-monorepo-workaround-plugin\index.js
  121:57  error  'from' is defined 
but never used. Allowed unused args must match /^_/u  @typescript-eslint/no-unused-vars

@nzakas nzakas requested a review from a team as a code owner May 23, 2024 18:31
@nzakas nzakas requested review from Jolg42 and removed request for a team May 23, 2024 18:31
@CLAassistant
Copy link

CLAassistant commented May 23, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

socket-security bot commented May 23, 2024

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

'@typescript-eslint': typescriptEslint,
jest,
'simple-import-sort': simpleImportSort,
import: fixupPluginRules(_import),
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! That's interesting, because I identified this as a blocker recently and this unblocks it.

(In #24148 (review))

@Jolg42 Jolg42 self-assigned this May 23, 2024
@Jolg42 Jolg42 added this to the 5.15.0 milestone May 23, 2024
@Jolg42
Copy link
Contributor

Jolg42 commented May 23, 2024

Thanks a lot @nzakas! 🙌🏼

I think we can ignore these errors for the blaze utils and rename from to _from for nextjs-monorepo-workaround-plugin.

Could you do that? I could not push a commit because of a permission error.

Also, we would need to have the CLA signed to be able to merge the PR later. Could you check this in the message above?

Ping @millsp for info and for the blaze utils where we need to ignore the eslint errors.

@millsp
Copy link
Member

millsp commented May 23, 2024

Thanks @nzakas for taking the time, this is very much appreciated.

@Jolg42 @nzakas these eslint errors should be ignored as it is intentional in the implementation.

C:\Users\nzaka\projects\third-party\prisma\helpers\blaze\omit.ts      
  11:45  error  Unexpected constant truthiness on the left-hand side of a `&&` expression  no-constant-binary-expression
  17:45  error  Unexpected constant truthiness on the left-hand side of a `&&` expression  no-constant-binary-expression

C:\Users\nzaka\projects\third-party\prisma\helpers\blaze\pick.ts      
  11:45  error  Unexpected constant truthiness on the left-hand side of a `&&` expression  no-constant-binary-expression
  17:45  error  Unexpected constant truthiness on the left-hand side of a `&&` expression  no-constant-binary-expression

and the variable producing an error here can be prefixed with an underscore to suppress the error.

C:\Users\nzaka\projects\third-party\prisma\packages\nextjs-monorepo-workaround-plugin\index.js
  121:57  error  'from' is defined 
but never used. Allowed unused args must match /^_/u  @typescript-eslint/no-unused-vars

@nzakas
Copy link
Contributor Author

nzakas commented May 23, 2024

CLA signed.

Updated the config file to ignore the blaze directory and renamed from to _from.

Comment on lines +55 to +62
...compat.extends(
'eslint:recommended',
'plugin:@typescript-eslint/eslint-recommended',
'plugin:@typescript-eslint/recommended',
'plugin:@typescript-eslint/recommended-requiring-type-checking',
'plugin:prettier/recommended',
'plugin:jest/recommended',
),
Copy link
Contributor

Choose a reason for hiding this comment

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

I see only one problem in this part. eslintrc will be long not supported and not maintened again. The compat "transforms" the configuration to support eslintrc old configuration. In this case i really want the best thing is to adapt fully to new eslint format.

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's a good approach in the long run. For right now, there are still too many plugins that don't officially support the new config format, so we need this. You can always revisit later as plugins are updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

TypeScript eslint now support Eslint v9. Here's a explain typescript-eslint/typescript-eslint#8211 (comment)

Prettier too have a Eslint v9 support. Here's a merged PR prettier/eslint-plugin-prettier#616

And you can see all supported parsers/plugins in the issue 18391

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, I maintain ESLint, so I'm aware. :)

This is just meant to be a courtesy PR to help get you started. This was generated by our new migration tool so there are some inefficiencies, but you can modify this however you'd like.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Now i understand everything, thanks for the explain

Comment on lines +25 to +27
{
files: ['**/*.ts'],
},
Copy link
Contributor

Choose a reason for hiding this comment

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

With the v9 release eslint give's a way to define rules for specific file extensions. in this block you have only a file extensions without inserting rules and others things. Are that really working good?

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. This is telling ESLint to search for **/*.ts files by default. This replaces the --ext .ts command line flag. You can optionally include additional configuration if you want, but the primary focus of this entry is to ensure that TypeScript files are search for by default.

@Jolg42 Jolg42 modified the milestones: 5.15.0, 5.16.0 Jun 4, 2024
Copy link
Contributor

@Jolg42 Jolg42 left a comment

Choose a reason for hiding this comment

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

Thanks a lot 🙌🏼

@Jolg42 Jolg42 merged commit a7dda70 into prisma:main Jun 4, 2024
214 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants