Skip to content

T98: Configure ESLint #135

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

Merged
merged 21 commits into from
Dec 12, 2024
Merged

T98: Configure ESLint #135

merged 21 commits into from
Dec 12, 2024

Conversation

Dennull
Copy link
Contributor

@Dennull Dennull commented Nov 4, 2024

Related Issues

Resolves #98 (finally!)

Summary

We finally set up ESLint, better late than never lol 😅. You can run the linter using the npx eslint command, and automatically fix linting issues using npx eslint --fix

Changes Made

ESLint was configured along with some extra plugins. I ended up going with the newest version of ESLint, v9, because v8 support ended recently. More details can be seen on the ESLint docs. All file changes made in this PR were to fix linting errors, no logic or UI changes were made

Here are some plugins that were installed:

  • eslint-plugin-prettier was installed to flag formatting during linting. Many formatting issues can be automatically fixed by running npx eslint --fix. It will help keep our formatting more tidy and consistent
  • eslint-plugin-import-x was installed to help sort our imports. Imports that are out of order or not imported properly will be flagged by the linter
  • eslint-plugin-react was configured to sort our props alphabetically

I also wanted to install eslint-plugin-react-hooks, but their ESLint v9 support isn't very good yet.. They technically support ESLint v9 as of this PR as can be seen here, but they don't have an easy way to use the new flat config system yet. There is an open issue about supporting flat config, but it hasn't been closed yet. The docs haven't been updated yet either, so I decided to just leave this one out until they give an update

Screenshots

No additional screenshots

Testing Instructions

No additional testing instructions

Special Notes for Your Reviewer(s)

No additional notes

@Dennull Dennull added the tech debt Refactors required label Nov 4, 2024
@Dennull Dennull requested a review from NasihNazeem November 4, 2024 01:43
@Dennull Dennull self-assigned this Nov 4, 2024
Copy link
Contributor

@NasihNazeem NasihNazeem left a comment

Choose a reason for hiding this comment

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

Great job putting this together! I left a few comments/questions before merging this through.

I love the changes though, it looks much cleaner than it was before

@Dennull Dennull requested a review from NasihNazeem November 17, 2024 02:32
Copy link
Contributor

@NasihNazeem NasihNazeem left a comment

Choose a reason for hiding this comment

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

Good work so far! I was testing out the linting command and there was a minor warning I found, so I suggested a change for that. There was also a random "bug" I found that I explained in another comment where I reference the suggested solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I tried running the command with the provided solution (settings object), I noticed that the command took quite some time, so I chat gpt'd reasons as to why it may be, and I guess I concluded with the idea that when I run the command in the root directory, it goes through all folders and files. I'm not sure if that's the reason why it takes so long, but I tested it by forcing the linting to only work on the src folder and it wound up being alot quicker

npx eslint --fix src/

Do you find a similar issue on your end or is this isolated to my computer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do find it kind of slow (like 10 seconds slow), but when I run npx eslint --fix src/ the time is about the same for me. I suspected that maybe it was linting node modules but by default eslint ignores node modules. I also ran npx eslint --debug and confirmed so that node modules wasn't being linted

I guess eslint-plugin-prettier adds some overhead, but when I tried removing the it the time was still was >5 seconds, so I think it's also eslint-plugin-import-x. Some files just take a while to lint with the plugins. You can try running npx eslint --debug yourself to see how long stuff takes to run, I'm open to any suggestions. Given that we're probably only running this once in a while I'm inclined to move forward and work on continue working on improving the performance later though, let me know if you agree

Copy link
Contributor Author

@Dennull Dennull Dec 6, 2024

Choose a reason for hiding this comment

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

Okay, as discussed offline, it turns out the issue was that the linter was looking through your build folder. I managed to find out why my config to ignore the build directory wasn't working and fixed it. Give it a shot and see if that stops the linter from going forever 👍

@Dennull Dennull requested a review from NasihNazeem November 24, 2024 18:39
Copy link
Contributor

@NasihNazeem NasihNazeem left a comment

Choose a reason for hiding this comment

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

I reviewed all changed files and nothing popped out at me in terms of a potential bug. I did notice an outdated PaperProps in PostMenu.tsx that should be changed to a slotProps but I think I'll keep that in mind for the next PR. It's not breaking anything.

I also tested out my eslint and it wasn't working initially, I ran a --debug and tried again and it worked as expected! I'm not sure what the --debug did though because it ran through a ton of files with logging going past the accessible amount of lines in my terminal.

We can finally merge this, let's go!!!

@Dennull Dennull merged commit 21a5f98 into main Dec 12, 2024
1 check passed
@Dennull Dennull deleted the T98-Configure-ESLint branch December 12, 2024 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech debt Refactors required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set up ESLint on frontend
2 participants