-
Notifications
You must be signed in to change notification settings - Fork 957
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
Conversation
cc @MattPereira could you check if this solves your problem mentioned in #901 ? |
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. |
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? |
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
![]() But if you think v8 is better we can discuss it and probably rewrite to v8 |
"build": "yarn lint && next build", | ||
"serve": "next start", | ||
"lint": "next lint", | ||
"lint": "eslint .", |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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 Demo video :Screen.Recording.2024-08-08.at.11.34.38.AM.movBtw was debugging with @MattPereira and seems like if you checkout before the #875 i.e to #869 :
It does seem to solve the problem for him, maybe something to do with prettier update |
also, eslint v8 will not be maintained in two months, see top of the page https://eslint.org/docs/v8.x/
Let's wait for others and see how it works. Looks like yes, you config is different
It works for me too. But reverting is not so simple. Changing back versions of eslint/prettier libs from |
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 |
Description
Fixes Typescript-eslint and updates eslint related packages
to test add this to any of the components:
also I removed this rule
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 filesSee 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