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

chore(website): configure eslint config similiar to asyncapi website #2031

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

jerensl
Copy link
Contributor

@jerensl jerensl commented Jun 10, 2024

Description

Added eslint automatic fix for modelina-website and added modelina-website to eslint ignore in parent project, so no conflicting rule in the future when we start migrating the same rule used in async website as mentioned here

Related Issue

This PR is being made as mentioned here

Copy link

netlify bot commented Jun 10, 2024

Deploy Preview for modelina canceled.

Name Link
🔨 Latest commit bc8e712
🔍 Latest deploy log https://app.netlify.com/sites/modelina/deploys/667a6a5bbcbe6700081d572b

Copy link

sonarcloud bot commented Jun 10, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@devilkiller-ag devilkiller-ag left a comment

Choose a reason for hiding this comment

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

@jerensl, you can also add the eslint and prettier rules from the asyncapi website in this PR and update the PR title accordingly.

@coveralls
Copy link

coveralls commented Jun 11, 2024

Pull Request Test Coverage Report for Build 9448551980

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.303%

Totals Coverage Status
Change from base Build 9382165682: 0.0%
Covered Lines: 5996
Relevant Lines: 6327

💛 - Coveralls

@jerensl
Copy link
Contributor Author

jerensl commented Jun 12, 2024

@jerensl, you can also add the eslint and prettier rules from the asyncapi website in this PR and update the PR title accordingly.

@devilkiller-ag After doing some work I figured out this doesn't work because the next cli makes us dirty here, basically next lint did something different with eslint, it doesn't allow us to have multiple configurations for eslint and always counted the root directory project eslint rule as mentioned in this NextJS discussion, this resulted of conflicting rule with the root directory project.

For the solution, I think it would be better to migrate the website as new repositories and use submodules for this repository

@jonaslagoni what is your opinion about this?

@jonaslagoni
Copy link
Sponsor Member

For the solution, I think it would be better to migrate the website as new repositories and use submodules for this repository

At the moment that wont be an option, too complex of a setup for now.

@devilkiller-ag After doing some work I figured out this doesn't work because the next cli makes us dirty here, basically next lint did something different with eslint, it doesn't allow us to have multiple configurations for eslint and always counted the root directory project eslint rule as mentioned in this vercel/next.js#36440, this resulted of conflicting rule with the root directory project.

It should be possible to create a website-only eslint configuration even through the next cli, if the next cli is even needed to lint the files.

@jerensl jerensl changed the title chore(website): add nextjs eslint automatic fix on website chore(website): configure eslint config similiar to asyncapi website Jun 18, 2024
@jerensl
Copy link
Contributor Author

jerensl commented Jun 19, 2024

@devilkiller-ag, I made adjusted to ignore next/lint and use only eslint for modelina-website. The next step is to run the fix command, I tested it on my local device, which changes all over the code in the website and leftover ~100(mostly minor changes) errors that need to be fixed manually

@devilkiller-ag
Copy link
Member

devilkiller-ag commented Jun 19, 2024

@devilkiller-ag, I made adjusted to ignore next/lint and use only eslint for modelina-website. The next step is to run the fix command, I tested it on my local device, which changes all over the code in the website and leftover ~100(mostly minor changes) errors that need to be fixed manually

@jerensl what type of errors are those?

@jerensl
Copy link
Contributor Author

jerensl commented Jun 19, 2024

@devilkiller-ag, I made adjusted to ignore next/lint and use only eslint for modelina-website. The next step is to run the fix command, I tested it on my local device, which changes all over the code in the website and leftover ~100(mostly minor changes) errors that need to be fixed manually

@jerensl what type of errors are those?

Mostly unused variable and hoisting

Terminal.mp4

@jerensl
Copy link
Contributor Author

jerensl commented Jun 20, 2024

Hi @devilkiller-ag if you don't mind I changed the entire website codebase, I made the latest commit for fixing around 100 eslint errors, which left around ~90 warnings related to jsdoc for documentation purposes

@devilkiller-ag
Copy link
Member

Hi @devilkiller-ag if you don't mind I changed the entire website codebase, I made the latest commit for fixing around 100 eslint errors, which left around ~90 warnings related to jsdoc for documentation purposes

Okay, No problem!

@coveralls
Copy link

coveralls commented Jun 22, 2024

Pull Request Test Coverage Report for Build 9626634076

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.304%

Totals Coverage Status
Change from base Build 9611843317: 0.0%
Covered Lines: 5997
Relevant Lines: 6328

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 22, 2024

Pull Request Test Coverage Report for Build 9627168389

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.304%

Totals Coverage Status
Change from base Build 9626689374: 0.0%
Covered Lines: 5997
Relevant Lines: 6328

💛 - Coveralls

.next
/**/*.css
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

@jerensl jerensl Jun 23, 2024

Choose a reason for hiding this comment

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

Unfortunately, ts parser cannot do something unless it's in typescript format, unless we migrate babel parser it can be included on eslint, for now it's better just to ignore it

@jerensl
Copy link
Contributor Author

jerensl commented Jun 23, 2024

@devilkiller-ag, I want to mention here too, with the deprecating of the majority eslint formatting rule, It's been clear they're not buying into the idea of becoming both formatter and linter simultaneously. So I propose a solution to make separation between both tools which we are using prettier for formatter and eslint as a linter

https://eslint.org/blog/2023/10/deprecating-formatting-rules/

@devilkiller-ag
Copy link
Member

@devilkiller-ag, I want to mention here too, with the deprecating of the majority eslint formatting rule, It's been clear they're not buying into the idea of becoming both formatter and linter simultaneously. So I propose a solution to make separation between both tools which we are using prettier for formatter and eslint as a linter

https://eslint.org/blog/2023/10/deprecating-formatting-rules/

Yeah this seems important to me. We should have this separation. Would you like to work on this @jerensl?

@coveralls
Copy link

coveralls commented Jun 25, 2024

Pull Request Test Coverage Report for Build 9657860438

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.304%

Totals Coverage Status
Change from base Build 9642600004: 0.0%
Covered Lines: 5997
Relevant Lines: 6328

💛 - Coveralls

@jerensl
Copy link
Contributor Author

jerensl commented Jun 25, 2024

Yeah this seems important to me. We should have this separation. Would you like to work on this @jerensl?

Sure

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.

None yet

4 participants