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 lint configuration #1219

Merged
merged 2 commits into from
Oct 31, 2024
Merged

Fix lint configuration #1219

merged 2 commits into from
Oct 31, 2024

Conversation

AliceR
Copy link
Member

@AliceR AliceR commented Oct 28, 2024

Related Ticket: {link related ticket here}

Description of Changes

My local dev setup / code editor setup wasn't working with format on save and the specified rules in .vscode/settings.json.sample. I noticed that we have the prettier rules disabled for all .ts/.tsx files in our .eslint configuration.
Additionally, we have the prettier rules specified in both the .prettierrc and the .eslintrc, which is redundant and confusing.
I enabled the prettier rules for .ts/.tsx files and removed the redundancy. After that, I ran yarn lint:scripts --fix to automatically fix the lint issues across the codebase, now that we have the rules enabled. Unfortunately, it turns out that this will give a few more issues that needs attention.
Before going deeper into fixing these, I want to ask the team:

  • how have you worked with this configuration? It was set up like this for about 2 years, so maybe I am not seeing how to configure it correctly on my end?
  • how would you like to go about this going forward? Clean up the configuration, fix the issues, or leave as is?
  • it seems you arrived at a cleaner setup for next-veda-ui, should we aim to get the same configuration here?

Notes & Questions About Changes

Opened a draft for team discussion

Validation / Testing

See if you can use formatOnSave to automatically correct the line indentation in your cloned codebase. Run yarn lint and check for errors.

Copy link

netlify bot commented Oct 28, 2024

Deploy Preview for veda-ui ready!

Name Link
🔨 Latest commit 668ab66
🔍 Latest deploy log https://app.netlify.com/sites/veda-ui/deploys/672356d65e7c64000994852b
😎 Deploy Preview https://deploy-preview-1219--veda-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@AliceR AliceR requested a review from dzole0311 October 28, 2024 11:08
@AliceR
Copy link
Member Author

AliceR commented Oct 29, 2024

We may also want to add the rule "import/no-unused-modules": [1, { "unusedExports": true }], to our eslint configuration, which will help us to detect dead code

@hanbyul-here
Copy link
Collaborator

hanbyul-here commented Oct 29, 2024

Thanks for working on cleaning up our lint configuration! Our lint configuration had been accumulated over the years, and could use some clean-ups. (I have to confess that I have been putting the rules as I see the need, without thinking holistically.)

But I am a bit confused

  1. How I was able to do 'format on save' with my editor (vscode) so far 🤔 Could it be because I fed our eslint configuration directly to my editor setup? (and not caring about prettier?)
  2. imo, it would be ideal if we can separate the issues - like if this PR's original intention is to address the issue of respecting prettier rule, let's just do that for now.
  3. I also see that this set-up actually changes the rule - One thing I noticed is how we type the interface. I remember we used semicolons for interface type declarions (ex. interface Example {foo: string; bar: string;}) but now it uses comma as far as I observe? (ex. interface Example {foo: string, bar: string;}) - Is this intentional change? If not, I wonder if this is somehow related to trailing comma rule of prettier 🤔
  4. I believe that we have no-unused-vars rule set up, (this probably is included in one of the recommended rules we are using as a plug-in) and the lint breaks if this rule doesn't get respected. Did you see a different thing? edit: OH I misread it, it is unused export, not unused vars 💦

@AliceR
Copy link
Member Author

AliceR commented Oct 30, 2024

Thanks for your comment, @hanbyul-here !

ESLint and Prettier serve different purposes and can complement each other:

  • ESLint is a static code analysis tool that identifies and reports on code patterns, such as unused variables, incorrect syntax, or potential bugs.
  • Prettier is a code formatter that ensures a consistent style in our code. It automatically formats code according to predefined style rules, such as indentation, line length, and spacing.

Typically, they are set up so that Prettier is integrated into the ESLint configuration using eslint-plugin-prettier. This plugin runs Prettier as an ESLint rule, meaning that code formatting issues will be reported as ESLint errors. Using eslint-config-prettier in the extends section (e.g., "extends": ["plugin:prettier/recommended"]) disables all ESLint rules that conflict with Prettier, preventing overlapping responsibilities.

In our VSCode settings.json, we suggest using "dbaeumer.vscode-eslint" as the default formatter, which is expected when we use Prettier via ESLint. However, if we disable Prettier for our TypeScript files, the format on save will still work, but only for ESLint rules—not for Prettier's formatting. You may notice changes to quotes or commas, but indentation may not be affected unless you select the Prettier extension as the default formatter, which would bypass our linting rules.

I believe we should agree on how to set up linting for this project. Here are three paths we can take:

A) Keep everything as is and work around it locally—this means we might miss out on either Prettier or ESLint when using format on save.

B) Enable Prettier to start throwing warnings and fix them as we go, following the principle of leaving the code we touch cleaner than we found it.

C) Clean up the linting rules in this project to adopt a completely new style. We could simplify a lot by following the “recommended” rules, run fixes across the repository, review any remaining issues, and use those new rules moving forward.

How do you all feel about these options? Which approach would you prefer? I’d love to hear your thoughts!

@dzole0311
Copy link
Collaborator

B) Enable Prettier to start throwing warnings and fix them as we go, following the principle of leaving the code we touch cleaner than we found it.

I think I prefer this option, enabling prettier to throw warnings and progressively format only the files that we touch in our PRs, without reformatting the entire codebase, or running a full fix all. Also we will have a better overview of what we change in each PR and adapt some of the rules if needed.

I also see that this set-up actually changes the rule - One thing I noticed is how we type the interface. I remember we used semicolons for interface type declarions (ex. interface Example {foo: string; bar: string;}) but now it uses comma as far as I observe? (ex. interface Example {foo: string, bar: string;}) - Is this intentional change? If not, I wonder if this is somehow related to trailing comma rule of prettier 🤔

Yeah I think this might be related to prettier’s formatting rules and how delimiters are handled

@AliceR
Copy link
Member Author

AliceR commented Oct 30, 2024

Adding another thought:
With all the refactoring happening, this might be a good opportunity to reassess our linting setup. We could redefine the rules and apply our desired configuration to a new folder for the refactored components, while keeping the existing rules as overrides for the old scripts folder. What do you think about this approach? Could this work?

@hanbyul-here
Copy link
Collaborator

Thanks for going into details!

B) Enable Prettier to start throwing warnings and fix them as we go, following the principle of leaving the code we touch cleaner than we found it.

Yeah I think I will vote for B too.

Regarding separating the lint rules - imo, veda-ui has been trying not to diverge the code as much as possible for the refactored parts of the code ex.Very often, refactored code is a container component that uses existing code. It would be ideal if the same rule goes to the lint rule (and it is a bit challenging to picture for me to separate them in a way that we can put them in separate folders but please let me know if there is something I am missing or you have a more concrete idea). Can we take a gradual approach such as, - first we make the prettier rule effecitve, without changing the current lint setup, then add the rules as we see the fits?

@AliceR
Copy link
Member Author

AliceR commented Oct 30, 2024

Ok, I will need to figure out how I can get the prettier formatting work on my end... I am not fully understanding the configuration we have in place, I think

in .prettierrc. Also, added the typescript parser as override for .ts/.tsx files, which finally enabled me to correctly lint the typescript files.
@AliceR
Copy link
Member Author

AliceR commented Oct 31, 2024

Hi @hanbyul-here, @dzole0311 and others, I think I found the solution we want. I enabled prettier for typescript files with a warning. Also, I specified the parser to be "typescript" for those files, which finally enabled me to use format on save as I expect it to behave.

Could you please check out those changes and test the behavior in your code editor? Especially indentation was a frustrating issue for me. I believe it should now work.
We will get a few changes as we touch files in the future, as now the format on save will fix prettier issues that have previously been ignored. Running lint:scripts results in 1964 problems (0 errors, 1964 warnings), potentially fixable with the --fix option.

Note: I force-pushed the branch to keep the git history clean from my failed attempts :)

@AliceR AliceR marked this pull request as ready for review October 31, 2024 10:12
Copy link
Collaborator

@dzole0311 dzole0311 left a comment

Choose a reason for hiding this comment

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

I tried the changes locally and it works much better from what I had before in terms of indentation. Thanks @AliceR!

Copy link
Collaborator

@hanbyul-here hanbyul-here left a comment

Choose a reason for hiding this comment

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

Thanks for working on it!!! 🙇

@AliceR AliceR merged commit d4de243 into main Oct 31, 2024
14 checks passed
@AliceR AliceR deleted the fix-lint-configuration branch October 31, 2024 15:29
@AliceR AliceR mentioned this pull request Nov 11, 2024
AliceR added a commit that referenced this pull request Nov 12, 2024
## 🎉 Features
* Card image/description independent of hero image/description by
@dzole0311 in #1244

## 🚀 Improvements
* Cookie consent code cleanup by @snmln in
#1199 , and @hanbyul-here in
#1240 , and @hanbyul-here in
#1241
* Add ADR about design system change by @j08lue in
#890
* Update condition to run playwright tests on release branches by
@dzole0311 in #1228
* Update STYLE_GUIDE.md by @AliceR in
#1227
* Fix lint configuration by @AliceR in
#1219
* Add tests for the AOI feature specification by @AliceR in
#1216
* Set data catalog filters to be closed by default by @vgeorge in
#1243
* Update tsconfig and make nav interfaces exposable for consumption by
@sandrahoang686 in #1223

## 🐛 Fixes
* Hotfix to hide the external link badge from cards by @dzole0311 in
#1231

## New Contributors
* @vgeorge made their first contribution in
#1243

**Full Changelog**:
v5.9.0...v5.10.0
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.

3 participants