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

Update ts, eslint, prettier #963

Merged
merged 21 commits into from
Oct 26, 2024
Merged

Update ts, eslint, prettier #963

merged 21 commits into from
Oct 26, 2024

Conversation

rin-st
Copy link
Member

@rin-st rin-st commented Oct 11, 2024

Description

  • updated typescript and eslint/prettier related deps
  • removed @typescript-eslint/eslint-plugin from nextjs package.json since it's already included to eslint-config-next
  • explicit "plugins": ["@trivago/prettier-plugin-sort-imports"] to avoid warnings. From docs
  • added check-types script for hardhat
  • a little bit formatting/fix types and disabled eslint for a few places after updates

Note: eslint v9 is still not supported by nextjs, so I used last v8 version

To test take code snippet here

Additional Information

Related Issues

Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

Thanks @rin-st!!

Just added a couple of comments

Also in tailwind.config.js I see an error but on main it doesn't seem to appear:

Screenshot 2024-10-16 at 8 34 52 PM

Also maybe its good chance to solve this warnings in .eslintrc files ?

Screenshot 2024-10-16 at 8 29 54 PM

present at both nextjs and hardhat.


Also pushed a minor commit not related to this PR fixing the types of useScaffoldEventHistory

packages/nextjs/package.json Outdated Show resolved Hide resolved
packages/hardhat/package.json Outdated Show resolved Hide resolved
@rin-st
Copy link
Member Author

rin-st commented Oct 17, 2024

Thanks for the great comments!

Also maybe its good chance to solve this warnings in .eslintrc files ?

7c82a33

Also in tailwind.config.js I see an error but on main it doesn't seem to appear:

It turns out that by default next lint checks only some folders but not the full nextjs dir. I changed it 9599964, so all the nextjs folder is linted now, including tailwind.config.js

Plus:

  • Removed parser which is already included into eslint-config-next (parser) and eslint rule included to typescript-eslint:recommended (which again included to eslint-config-next)
  • I met bug: ESLint Error #901 again. I changed prettier file to .prettierrc.js and changed import, because of @trivago/prettier-plugin-sort-imports causes bug with underlines in vscode. It's a pity that I couldn't find it earlier, maybe bug: ESLint Error #901 wont even started

@technophile-04
Copy link
Collaborator

technophile-04 commented Oct 22, 2024

Thanks Rinat!!

It turns out that by default next lint checks only some folders but not the full nextjs dir. I changed it 9599964, so all the nextjs folder is linted now, including tailwind.config.js

Umm I didn't get this changes 😅, also maybe its bad to pass it whole dir? and we should keep its as it was earlier what next lint by defults lints? Since next team might have researched to put all necessary to lint and ignore unnecessary stuff?

Also I see // eslint-disable-next-line @typescript-eslint/no-require-imports to tailwind config so that it doesn't complain anymore and its solved, so we don't need 9599964?

I met #901 again. I changed prettier file to .prettierrc.js and trivago/prettier-plugin-sort-imports#131 (comment), because of @trivago/prettier-plugin-sort-imports causes bug with underlines in vscode. It's a pity that I couldn't find it earlier, maybe #901 wont even started

Ohh so #901 appeared again? Should we ask Matt to test again? Just in case because its #901 never happened to me so can't test :/

@rin-st
Copy link
Member Author

rin-st commented Oct 22, 2024

Umm I didn't get this changes 😅, also maybe its bad to pass it whole dir? and we should keep its as it was earlier what next lint by defults lints? Since next team might have researched to put all necessary to lint and ignore unnecessary stuff?

We need to decide if we care about files like tailwind.config.js. If not, we can use just default next lint but I'm voting for linting everything

so we don't need 9599964?

Without that change next lint doesn't find lint errors outside of that folders, for example in tailwind.config.js. So without linting all the nextjs folder I can't know if there's some lint error outside default folders. Same for gh-actions and lint-staged since they use next:lint and errors are not found. That's why after my first commits, error in tailwind.config.js was still existed

After 9599964 error is found by next:lint and hence by gh-actions. Also updated lintstagedrc b0fb91b , so js files are also listed

Am I missing something?

Ohh so #901 appeared again? Should we ask Matt to test again? Just in case because its #901 never happened to me so can't test :/

Yes, but it's fixed now.

@MattPereira hey, could you please test if #901 exists for you in this branch? (Checkout to branch -> yarn install -> (probably restart vscode) -> test code snippet from mentioned pr

@technophile-04
Copy link
Collaborator

If not, we can use just default next lint but I'm voting for linting everything

Umm I would vote for going with next lint default 😅 don't want to opinionated on it.

Also I am not sure about linitng JS files at gh actions / linstaged b0fb91b because people who are using JS believes in living on the edge? (move fast, break stuff is fine for them?), I think we are adding friction. Also I am not sure if people are actually using JS or not with SE-2 so lol I don't have strong opinion on this but slightly leaning towards not having lint checks and keep things as previous version

@rin-st
Copy link
Member Author

rin-st commented Oct 23, 2024

Umm I would vote for going with next lint default 😅 don't want to opinionated on it.

Also I am not sure about linitng JS files at gh actions / linstaged b0fb91b because people who are using JS believes in living on the edge? (move fast, break stuff is fine for them?), I think we are adding friction. Also I am not sure if people are actually using JS or not with SE-2 so lol I don't have strong opinion on this but slightly leaning towards not having lint checks and keep things as previous version

Ok, I reverted these changes, but we should remember then that linting works only for default next lint folders

@technophile-04
Copy link
Collaborator

Tysm Rinat!! Just texted Port to do final testing on this #901 (comment)

Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

Thanks @portdeveloper for test!!

@technophile-04 technophile-04 merged commit c7772e3 into main Oct 26, 2024
1 check passed
@technophile-04 technophile-04 deleted the update-lint-prettier-deps branch October 26, 2024 05:41
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.

2 participants