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

Update stylistic rules #187

Merged
merged 7 commits into from
Feb 5, 2024
Merged

Update stylistic rules #187

merged 7 commits into from
Feb 5, 2024

Conversation

reiv
Copy link
Member

@reiv reiv commented Feb 1, 2024

Fixes #184

Prohibit foo={true} on JSX tags, foo is enough as attribute
/* eslint react/jsx-boolean-value: ["warn", "never"] */

Enforce self-closing tags instead of having expanded empty tags <div></div> should be <div />
/* eslint react/self-closing-comp: "warn" */

Enforce expression bodies of arrow function () => { return "foo"; } should become () => "foo"
/* eslint arrow-body-style: ["warn", "as-needed"] */

Remove unnecessary fragments: <><div /></> becomes <div />
/* eslint react/jsx-no-useless-fragment: "warn" */

Note: All of these rules can also be fixed automatically.

[ ] Trim classNames (if possible) w-full h-12 (trailing space) becomes w-full h-12

This seems a bit tricky right now. prettier-plugin-tailwindcss used to have this feature but it was quickly reverted because it was trimming more than it should have.

See tailwindlabs/tailwindcss#7560

In lieu of other options, we would have to write our own rule. I found this (deprecated) plugin which maybe could be used for reference:
https://github.com/liferay/eslint-config-liferay/blob/master/plugins/eslint-plugin-liferay/lib/rules/trim-class-names.js

I'll make a new issue for that.


As we add more rules, I would like to stick to the convention that "stylistic" rules (i.e. those that do not catch bad runtime behavior) are marked as warnings instead of errors. We should still strive to have zero warnings in total and our CI usually enforces this; however, warnings are highlighted differently in IDEs and it is helpful to be able to differentiate them from "actual" errors. WDYT?

eslint/react.js Outdated Show resolved Hide resolved
@mvarendorff2
Copy link
Member

As we add more rules, I would like to stick to the convention that "stylistic" rules (i.e. those that do not catch bad runtime behavior) are marked as warnings instead of errors. We should still strive to have zero warnings in total and our CI usually enforces this; however, warnings are highlighted differently in IDEs and it is helpful to be able to differentiate them from "actual" errors. WDYT?

Yeah, I agree. I personally don't really mind much whether my editor shows it as red or yellow underlines but I can see how it reduces the mental strain to make sure to remove all "errors" from code before trying it out. Less stuff to have in the back of the mind is usually a good thing when working.

In lieu of other options, we would have to write our own rule.

There probably is a reason something like this doesn't exist already; I assume the near endless options for expansion, function calls, etc. in JSX attributes makes it really hard to write a good rule for that. But we could strive to set up something for simple strings without calls to variants / twMerge / clsx, that's true 👍

@mvarendorff2 mvarendorff2 merged commit bdd0ec1 into main Feb 5, 2024
@mvarendorff2 mvarendorff2 deleted the nitpicking branch February 5, 2024 09:39
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.

Miscellaneous improvements for React and Tailwind
2 participants