fix: typescript eslint + up eslint

rin-st commented 3 months ago

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

Additional Information

[x] I have read the contributing docs (if this is your first contribution)
[x] This is not a duplicate of any existing pull request

Related Issues

Fixes #901

technophile-04 commented 3 months ago

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

MattPereira commented 3 months ago

Hey thanks so much for working on this! :pray:

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

MattPereira commented 3 months ago

Also no warnings for missing useEffect dependencies

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

anton-karlovskiy commented 3 months ago

Also no warnings for missing useEffect dependencies

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

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 commented 3 months ago

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 https://github.com/vercel/next.js/issues/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 commented 3 months ago

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

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

technophile-04 commented 3 months ago

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 :

https://github.com/user-attachments/assets/c3ece56d-b9c8-4881-907b-ec7c1affd4ea

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 commented 3 months ago

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 commented 3 months ago

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

scaffold-eth / scaffold-eth-2