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: enable recommended ESLint plugin rules #681

Closed

Conversation

nschonni
Copy link
Contributor

@nschonni nschonni commented Aug 9, 2023

Turn on the recommended set of rules for the current plugins and turn off the currently failing rules. The rules should be enabled/fixed later.

🤖 Generated by Copilot at b950006

Summary

🔧✨♻️

This pull request improves the linting configuration for the project by updating the .eslintrc.json file and removing the eslintConfig property from the package.json file. These changes enhance the code quality, accessibility, and testing of the React app.

To make our code linting more pleasant
We removed eslintConfig from package.json
And updated .eslintrc.json with care
To fix some issues and make rules more fair
Now our code quality is more consistent

Walkthrough

  • Configure ESLint for React, React Hooks, and accessibility rules (link)
  • Move Jest plugin settings to test file overrides (link, link)
  • Simplify plugin loading and disable some recommended rules (link)
  • Remove redundant eslintConfig property from package.json (link)

@anth-volk
Copy link
Collaborator

@nschonni Apologies for the delay in addressing this PR. Would you mind providing a bit of context as to what/why things were failing? I joined a bit after this issue and am wondering if it's still relevant. Thanks so much.

@anth-volk
Copy link
Collaborator

@nikhilwoodruff Should we move ahead with this?

@nikhilwoodruff
Copy link
Contributor

Yep I think this is still worthwhile!

@abhcs
Copy link
Collaborator

abhcs commented Jan 4, 2024

I had not seen this PR before writing PRs #1080 and #1098 (sorry about the overlap!).

The first change in the new PRs is the use of the eslint-react-app config. This includes accessibility checks and optional jest rules (that I have not activated).

The second change is that I have not disabled any rules but instead fixed the errors in the code.

In light of these changes, we have to figure out which parts of this PR still apply or need modification. I can see that the Jest and jsx-a11y parts still apply but disabling is not needed for many of the rules.

@anth-volk
Copy link
Collaborator

Thanks for your input, @abhcs, I had meant to raise this with you early next week as I went through cleaning out a few almost-stale PRs. Since you teed it up, though, do you think there's value in separating out these recommended plugins as separate PRs? I'm anticipating that at least in a couple cases, there should be some pretty significant fixes that would have to be implemented should we activate them.

@abhcs
Copy link
Collaborator

abhcs commented Jan 5, 2024

"plugin:jest/recommended" has been enabled properly in this PR by fixing the errors.

"plugin:jsx-a11y/recommended" has been enabled too but by disabling some of the rules. I am not sure what rules lie in

"plugin:jsx-a11y/recommended" - "react-app" - {disabled rules in this PR}.

If this set is non-empty, then there is value in adding "plugin:jsx-a11y/recommended" with the disabled rules. For simplicity, we can ignore "plugin:jsx-a11y/recommended" in this PR and handle it separately. I agree with your assessment that enabling these rules will require significant effort.

@anth-volk
Copy link
Collaborator

anth-volk commented Jan 5, 2024

Because of the previous lint-related PRs, this PR is now also like 50% merge conflicts...

@abhcs What if instead we close this, then you open a new one enabling plugin:jest/recommended first; we merge that, then move to a11y with an aim toward actually meeting all of the rules (if you're willing to take on that herculean task)?

Alternatively, would you be willing to handle the merge conflicts of this PR, drop a11y, and ensure that jest/recommended still functions properly?

And what of the prettier plugin?

@abhcs
Copy link
Collaborator

abhcs commented Jan 5, 2024

"plugin:prettier/recommended" runs Prettier as an ESLint rule and reports differences as individual ESLint issues.

This would be valuable if we were using prettier through eslint but we are using prettier with eslint everywhere. So, it can be ignored.

How about the following suggestion? I can open a PR that enables the plugins a11y and jest but with the following offending rules disabled:

    "jsx-a11y/mouse-events-have-key-events": "off",
    "jsx-a11y/click-events-have-key-events": "off",
    "jsx-a11y/no-static-element-interactions": "off",
    "jsx-a11y/no-noninteractive-element-interactions": "off"

and with @nschonni 's fixes for jest.

Then, you can open four new issues -- one for each of the rules -- hopefully people will volunteer and fix the rules one by one.

@anth-volk
Copy link
Collaborator

@abhcs Thanks for the details on prettier, and that sounds like a great plan

@anth-volk
Copy link
Collaborator

Superseded by #1114. Closing.

@anth-volk anth-volk closed this Jan 5, 2024
@nschonni nschonni deleted the eslint-plugin-recommended branch January 5, 2024 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

4 participants