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

Add ESLint rule to wrap multiline JSX in parentheses #18302

Closed
wants to merge 4 commits into from

Conversation

koke
Copy link
Contributor

@koke koke commented Nov 6, 2019

Description

I noticed this is the prevalent style across the codebase but it's not currently enforced by ESLint.

I added react/jsx-wrap-multilines to the recommended set of rules.

I also changed the default configuration to ensure new lines after the parentheses. There were 18 violations with the default settings and 23 with the new-line setting.

The condition, logical, and prop configurations also seemed nicer to me, but enabling them bumped the violation count to 118, so it doesn't seem there's enough consensus on that one.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@koke koke force-pushed the add/eslint-jsx-wrap-multilines branch from 31ae4a2 to eef514d Compare November 6, 2019 10:25
@koke
Copy link
Contributor Author

koke commented Nov 6, 2019

cc @aduth since your past suggestion to do this was the only result when searching for this rule 😊

@gziolo
Copy link
Member

gziolo commented Nov 6, 2019

This requires a note in the CHANGELOG of the @wordpress/eslint-plugin as this is a new feature added.

@koke
Copy link
Contributor Author

koke commented Nov 6, 2019

Good call, I missed that one. Pushed in 1fbe05e

@gziolo
Copy link
Member

gziolo commented Nov 6, 2019

It got me thinking whether it will play nicely with Prettier, @jsnajdr opened #18048 which we should land sooner than later.

@koke
Copy link
Contributor Author

koke commented Nov 6, 2019

I tried that branch and cherry-picked the change to enable the rule. It doesn't seem to conflict (although I'm seeing some unrelated lint errors , but it's also not catching any violations.

I think if prettier is going to land soon, this PR just becomes superfluous, so it depends on the timeline for that.

@jsnajdr
Copy link
Member

jsnajdr commented Nov 7, 2019

Prettier inserts parens around multiline JSX expressions in most cases, so it shouldn't conflict with this ESLint rule.

But once Prettier gets merged, these formatting-only ESLint rules become superfluous -- the formatting is handled by AI and doesn't need to be checked by another AI 🙂 That's why my PR removes them and why there even exists a eslint-config-prettier package that disables formatting rules.

@etoledom
Copy link
Contributor

@koke - since the Prettier PR already landed, is this PR still relevant?

@gziolo
Copy link
Member

gziolo commented Jan 30, 2020

We still need to formal all files but it shouldn’t be an issue anymore. From now on, Prettier is the boss in terms of formatting 😃

@koke koke closed this Jan 31, 2020
@koke koke deleted the add/eslint-jsx-wrap-multilines branch January 31, 2020 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] ESLint plugin /packages/eslint-plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants