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: Fixed ESLint styling rules overriding Prettier styling rules #244

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

aryanprince
Copy link
Contributor

@aryanprince aryanprince commented Mar 18, 2024

Summary

This fixes the minor issue where ESLint's styling rules were overwriting the Prettier styling rules. Videos below showcase an example.

This PR is part of the Turborepo monorepo migration mentioned in #234.

EDIT: Comment below in this PR (here) showcases an example of the conflicting styling rules caused before this PR.

Changes

  • Removed 2 of ESLint's conflicting styling rules

Showcase

Before Changes

👇 Shows how the lint:fix script overrides changes done by format:fix

Before.Changes.mp4

After Changes

👇 ESLint (lint:fix) no longer conflicts with Prettier's (format:fix) styling changes now

After.Changes.mp4

@aryanprince
Copy link
Contributor Author

@hkirat Although it may seem this PR (#244) and #243 are tackling the same issue - this PR is purely to solve the conflicting styling rules that are causing issues in CI (for a lot of PRs), and locally when using Husky pre-commit hooks.

#243 is about resolving another issue and adds new features, while bringing a much larger diff/file changes.

I believe the best case scenario would be to first merge this PR to ensure our GitHub Actions CI and Husky pre-commit hooks continue to do their jobs effectively, before merging other issues.

This was referenced Mar 18, 2024
@TanmayDhobale
Copy link
Contributor

@aryanprince how u are u solving this issue just removing ?

  quotes: [2, 'single']
  indent: [2, 2

@aryanprince
Copy link
Contributor Author

@TanmayDhobale Good question, it was very annoying during dev cus of our Husky pre-commit hooks - and being very annoying to debug as well.

Video below shows an example of one of the conflicts caused by ESLint rule: indent: [2, 2]. You can see how running format:fix formats all files according to Prettier's conventions and our Prettier config file (.prettierrc). But running lint:fix then overwrites styling rules according to these 2 ESLint rules I just removed in this PR.

Adding to this, every time someone makes a PR/commit, our Husky pre-commit rules will first apply Prettier formatting using format:fix, and the next step is to lint the whole codebase using lint:fix. But then again, on every commit the ESLint styling rules will override any of Prettier's styling rules since both of these scripts run on every commit.

Showcasing.ESLint.and.Prettier.Conflict.mp4

Issue is having ESLint do any styling for us since Prettier already does a great job at that. It would be best to remove all of the ESLint styling rules we have to prevent any future conflicts. But the 2 rules I removed here were the only ones causing conflicts in this codebase currently.

@TanmayDhobale
Copy link
Contributor

@TanmayDhobale Good question, it was very annoying during dev cus of our Husky pre-commit hooks - and being very annoying to debug as well.

Video below shows an example of one of the conflicts caused by ESLint rule: indent: [2, 2]. You can see how running format:fix formats all files according to Prettier's conventions and our Prettier config file (.prettierrc). But running lint:fix then overwrites styling rules according to these 2 ESLint rules I just removed in this PR.

Adding to this, every time someone makes a PR/commit, our Husky pre-commit rules will first apply Prettier formatting using format:fix, and the next step is to lint the whole codebase using lint:fix. But then again, on every commit the ESLint styling rules will override any of Prettier's styling rules since both of these scripts run on every commit.

Showcasing.ESLint.and.Prettier.Conflict.mp4
Issue is having ESLint do any styling for us since Prettier already does a great job at that. It would be best to remove all of the ESLint styling rules we have to prevent any future conflicts. But the 2 rules I removed here were the only ones causing conflicts in this codebase currently.

yea bcs i try many times but without changing files code lint problem is still there. then i have to change the file code automatically u can see here ive try to solve this issue in #211 but then u solved , great

@devsargam
Copy link
Collaborator

@aryanprince @TanmayDhobale do u know about extending eslint with prettier. It might be somewhat better than what we are doing currently

ref = https://github.com/prettier/eslint-config-prettier

@aryanprince
Copy link
Contributor Author

aryanprince commented Mar 19, 2024

@devsargam I 100% agree it's a better solution and it's what I gravitated towards at first. That's what I use in most of my personal projects.

But I instead found out that I could solve the conflicts just by disabling the 2 ESLint styling rules for now, instead of introducing a new dependency (like eslint-prettier-config) for it.

@TanmayDhobale Feel free to include eslint-prettier-config in your next PR. I would still recommend you still open a PR (like #243 which you closed) as you were planning to add more new features, perhaps after this PR gets merged to ensure minimal conflicts.

@TanmayDhobale
Copy link
Contributor

@devsargam I 100% agree it's a better solution and it's what I gravitated towards at first. That's what I use in most of my personal projects.

But I instead found out that I could solve the conflicts just by disabling the 2 ESLint styling rules for now, instead of introducing a new dependency (like eslint-prettier-config) for it.

@TanmayDhobale Feel free to include eslint-prettier-config in your next PR. I would still recommend you still open a PR (like #243 which you closed) as you were planning to add more new features, perhaps after this PR gets merged to ensure minimal conflicts.

Not sure let's see

@devsargam
Copy link
Collaborator

@aryanprince there are a few edge cases where your approach might not work everytime.

But one thing, after removing those 2 options now eslint won't provide rules for indentation in this project. This issue solves a big problem but accidentally starts another small one

cc @TanmayDhobale please look into it!!!

@aryanprince
Copy link
Contributor Author

But one thing, after removing those 2 options now eslint won't provide rules for indentation in this project.

I agree. But we are better off with ESLint not providing rules for indentation, since Prettier will auto format the files anyways - especially with the Husky pre-commit hooks in place.

This issue solves a big problem but accidentally starts another small one

Could you provide some examples where this could start other problems? Currently, all of our other ESLint styling rules (in .eslintrc) don't seem to have any overlap with our Prettier styling rules (in .prettierrc) so we should be good.

@devsargam
Copy link
Collaborator

devsargam commented Mar 19, 2024

@aryanprince I don't remember much brother. But I did see some weird behaviour when it was a case for nested ternaries
Example

true ?
  true ?
     "yes" : "no"
  : "something"

I just know this because I raised an issue way earlier #23

@TanmayDhobale
Copy link
Contributor

after reviewing our current setup and the discussions around ESLint and Prettier conflicts, I suggest integrating eslint-config-prettier as a more sustainable solution. This approach will systematically disable all ESLint rules that could conflict with Prettier, ensuring a smoother workflow without needing to manually disable specific rules. It not only simplifies our setup but also enhances maintainability and minimizes potential issues down the road. let me know pls @devsargam @aryanprince

@devsargam
Copy link
Collaborator

@TanmayDhobale sure go on. For now @aryanprince solution seems fine as it's blocking the turoborepo migration

@aryanprince
Copy link
Contributor Author

aryanprince commented Mar 19, 2024

@devsargam @TanmayDhobale I fully agree, can totally see this happening.

Best case scenario would be to update our ESLint config to use eslint-prettier-config, and then run the linter across the entire project to fix any issues. But this will cause merge conflicts with every other open PR.


For now @aryanprince solution seems fine as it's blocking the turoborepo migration

True, this minimal ESLint config change is the smallest possible change I could do to ensure the Turborepo migration goes smoothly.

@devsargam
Copy link
Collaborator

yeah, linting is always tedious and should happen when the pr count is almost 0 else cause a lot of merge conflicts. Upto then you guys keep comitting with --no-verify flag which wont trigger husky

@TanmayDhobale
Copy link
Contributor

Agreed, let’s prioritize minimizing merge conflicts by holding off on sweeping lint changes until our PR count is low. Using --no-verify temporarily seems like the best path forward. We’ll circle back to eslint-config-prettier integration later.

@aryanprince aryanprince mentioned this pull request Mar 19, 2024
@TanmayDhobale
Copy link
Contributor

@devsargam yoo whtats ur discord ? or maybe check ur twitter dm once

@hkirat hkirat merged commit 612fa7f into code100x:main Apr 2, 2024
1 check passed
@aryanprince aryanprince deleted the chore/fixed-eslint-config branch April 3, 2024 09:04
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.

4 participants