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

fix: typescript eslint + up eslint #904

Closed
wants to merge 8 commits into from
Closed

Conversation

rin-st
Copy link
Member

@rin-st rin-st commented Aug 5, 2024

Description

Fixes Typescript-eslint and updates eslint related packages

to test add this to any of the components:

const t = 5; // 't' is assigned a value but never used.eslint[@typescript-eslint/no-unused-vars](https://typescript-eslint.io/rules/no-unused-vars)

type Test = {}; // 'Test' is defined but never used.eslint[@typescript-eslint/no-unused-vars](https://typescript-eslint.io/rules/no-unused-vars) , 
// The `{}` ("empty object") type allows any non-nullish value, including literals like `0` and `""`.
- If that's what you want, disable this lint rule with an inline comment or configure the 'allowObjectTypes' rule option.
- If you want a type meaning "any object", you probably want `object` instead.
- If you want a type meaning "any value", you probably want `unknown` instead.eslint[@typescript-eslint/no-empty-object-type](https://typescript-eslint.io/rules/no-empty-object-type)

// @ts-nocheck // Do not use "@ts-nocheck" because it alters compilation errors.eslint[@typescript-eslint/ban-ts-comment](https://typescript-eslint.io/rules/ban-ts-comment)

const age: any = "seventeen"; // any is allowed, no error
console.log(age);

also I removed this rule

"prettier/prettier": [
      "warn",
      {
        "endOfLine": "auto"
      }
    ]

because looks like it's redundant, also I think default ls is better. You can check how it works by removing last line in any of checked files

See also
https://typescript-eslint.io/getting-started/
https://typescript-eslint.io/rules/no-unused-vars#how-to-use
https://typescript-eslint.io/rules/no-explicit-any
https://typescript-eslint.io/rules/ban-ts-comment/
https://github.com/prettier/eslint-plugin-prettier?tab=readme-ov-file#configuration-new-eslintconfigjs

Additional Information

Related Issues

Fixes #901

@rin-st rin-st mentioned this pull request Aug 5, 2024
@technophile-04
Copy link
Collaborator

technophile-04 commented Aug 6, 2024

cc @MattPereira could you check if this solves your problem mentioned in #901 ?

@MattPereira
Copy link
Contributor

MattPereira commented Aug 6, 2024

Hey thanks so much for working on this! 🙏

The ESLint server isnt throwing error anymore and unused vars / import warnings look good

But when i try to make a commit its hanging indefinitely on yarn next:lint --fix --file app/page.tsx

image

@MattPereira
Copy link
Contributor

MattPereira commented Aug 6, 2024

Also no warnings for missing useEffect dependencies

image

Here's what it looks like on older scaffold build I started in ~March, 2024
image

@anton-karlovskiy
Copy link

anton-karlovskiy commented Aug 6, 2024

Also no warnings for missing useEffect dependencies

image

Here's what it looks like on older scaffold build I started in ~March, 2024 image

I've started using https://github.com/scaffold-eth/scaffold-eth-2. And to be honest, my first experience with this tool is not good.
I think there should be warnings for missing useEffect dependencies as it could lead to serious bugs. Actually, I've come across such bugs these days and it's really hard to find issues with useEffect because of no warnings for missing useEffect dependencies.
Could you possibly fix that issue as soon as possible?

cc @technophile-04 @MattPereira @rin-st

@technophile-04
Copy link
Collaborator

Umm @rin-st I don't feel confident about this PR, seems like eslint v9 / flat config doesn't go well with NextJs yet checkout vercel/next.js#64409. People seems to suggest hacky workaround which I don't feel we should jump in and maybe wait until NextJS officially support eslint v9 and new flat config?

@rin-st
Copy link
Member Author

rin-st commented Aug 7, 2024

I tried a guide from this message + last message in thread + some updates and everything worked fine

I think it's better to use v9 since

  • we don't know when nextjs will officially support eslint v9, it can take months
  • eslint v8 can conflict with related packages (configs/plugins/types etc), we need to check which ones are for v8 and which ones are for v9
image

But if you think v8 is better we can discuss it and probably rewrite to v8

Comment on lines +8 to +10
"build": "yarn lint && next build",
"serve": "next start",
"lint": "next lint",
"lint": "eslint .",
Copy link
Collaborator

Choose a reason for hiding this comment

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

So basically instead of running next lint we are now doing our own lint, since next lint it broken right with v9?

Not sure if this is gonna affect us vercel/next.js#64409 (comment) or any user future of SE-2

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, see also vercel/next.js#64409 (comment). We can wait for next answers though

@technophile-04
Copy link
Collaborator

I think it's better to use v9 since
we don't know when nextjs will officially support eslint v9, it can take months
eslint v8 can conflict with related packages (configs/plugins/types etc), we need to check which ones are for v8 and which ones are for v9

Yup agree but it would have been great if Next natively support it because if not there are some edge cases like this which will pop up.

But yeah I am not completely against it and kind of 49, 51% and we could go ahead with this PR if it nicely solves all the problem.

Also its wired that eslint works nicely for me on main branch but it breaks on this branch, same problem as you all were having on main but I have it on this branch :(. Might be something to do with my config we could totally ignore it!

Demo video :
Screen.Recording.2024-08-08.at.11.34.38.AM.mov

Btw was debugging with @MattPereira and seems like if you checkout before the #875 i.e to #869 :

git checkout a78be8761e1d039218cccf57d60a4de56261d859 && yarn install

It does seem to solve the problem for him, maybe something to do with prettier update

@rin-st
Copy link
Member Author

rin-st commented Aug 8, 2024

I think it's better to use v9 since
we don't know when nextjs will officially support eslint v9, it can take months

also, eslint v8 will not be maintained in two months, see top of the page https://eslint.org/docs/v8.x/

Also its wired that eslint works nicely for me on main branch but it breaks on this branch, same problem as you all were having on main but I have it on this branch :(. Might be something to do with my config we could totally ignore it!

Let's wait for others and see how it works. Looks like yes, you config is different

Demo video :
Btw was debugging with @MattPereira and seems like if you checkout before the #875 i.e to #869 :

git checkout a78be8761e1d039218cccf57d60a4de56261d859 && yarn install

It does seem to solve the problem for him, maybe something to do with prettier update

It works for me too. But reverting is not so simple. Changing back versions of eslint/prettier libs from main doesn't work for me, so I'm tend to v9

@technophile-04
Copy link
Collaborator

Tysm @rin-st for tinkering and exploring this path! We could surely come back to this PR soon or when next support v9. Closing this for now

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.

bug: ESLint Error
4 participants