Eslint Plugin: Lint code formatting.

[epiqueras]

This PR follows #18048 by:

1. Adding eslint-plugin-prettier to @wordpress/eslint-plugin to enforce formatting on-lint.
2. Adding the missing React prettier eslint configuration to @wordpress/eslint-plugin.
3. Making formatting run right before the linting pre-commit hook to avoid it failing due to formatting errors.
4. Formatting the entire codebase.

eslint-plugin-prettier was chosen over options like prettier-eslint because we don't run eslint --fix in hooks as we'd rather leave certain linting errors to be resolved or ignored at the author's discretion. We also don't apply any additional formatting with eslint over prettier, so the overhead would be unnecessary.

eslint-plugin-prettier was chosen over options like prettier --check because it's nice to see format errors as you type as it leads you to write code with a more optimal auto-formatted output and it avoids issues like comment directives being moved out of place by prettier and the author not realizing it.

[ItsJonQ]

🚀 from me! Tested locally and is working as expected. Left comments earlier in discussion.

[ntwb]

Had a quick glance, looks good, but near impossible to review ;)

Hiding white space changes helps:
https://github.com/WordPress/gutenberg/pull/19963/files?utf8=%E2%9C%93&diff=unified&w=1

[mkevins]

I didn't review everything 😅 but this scratches an itch I've had moving ? and : to the start of the line for multi-line ternary operators. 👍 Thanks for this work!

[gziolo]

Had a quick glance, looks good, but near impossible to review ;)

You can go commit by commit to see changes applied to the ESLint plugin. I guess the last one with formatting, will need to be discarded and applied again due to all those conflicts.

We also need the announcement post to let all contributors know what to do about all open patches. One thing that might work is:

• checkout your branch
• temporarily clone Gutenberg to another folder and set it up for master branch
• run npm run format-js ../gutenberg/ (or something like that)
• commit changes in your branch
• rebase with master

[gziolo]

Thank you @epiqueras for taking care of it 💯

I removed locally the last commit with all formatting and executed npm run lint-js to see how this integration works:

✖ 13376 problems (13371 errors, 5 warnings)
  13371 errors and 0 warnings potentially fixable with the `--fix` option.

It reports a ton of violations so it must work properly.
I was surprised seeing that ESLint also tells you that it can fix those errors and it works. A subsequent npm run format-js updated only one file (I need to double-check if it is properly linted).

One observation, those errors reported can be noisy and often not easy to get:

Let's see how it all works in action as it might be a completely different experience once we start with 0 errors and pre-commit hook enabled for formatting 👍

The CHANGELOG file for @wordpress/eslint-plugin needs to be updated to reflect changes from this PR in the New Features section:
https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/CHANGELOG.md#new-features

Optionally, it might be also helpful to explain how Prettier and ESLint work together in the recommended ruleset, this is what we have https://github.com/WordPress/gutenberg/tree/master/packages/eslint-plugin#usage at the moment:

The recommended preset will include rules governing an ES2015+ environment, and includes rules from the eslint-plugin-jsx-a11y, eslint-plugin-react, and eslint-config-prettier projects.

[jsnajdr]

I was surprised seeing that ESLint also tells you that it can fix those errors and it works.

The plugin works by formatting the source with Prettier, diffing it with the original source, and reporting all differences as lint errors. These errors can be fixed very simply by applying the diffs.

One observation, those errors reported can be noisy and often not easy to get:

These errors are basically diff hunks (the lines with +++ and --- prefixes you can see in a typical diff) that are formatted into an one line sentence. No wonder they are difficult to read by human 🙂

Optionally, it might be also helpful to explain how Prettier and ESLint work together in the recommended ruleset

Yes, this can always use more explanation 🙂 eslint-config-prettier just disables conflicting rules and doesn't do any actual formatting at all. eslint-plugin-prettier formats the file with Prettier and reports the diff hunks as ESLint errors. I never used prettier-eslint and looking at the README, don't find it very interesting.

Thanks @epiqueras for moving the Prettier integration forward! The eslint-plugin-prettier plugin is used by the Prettier codebase itself, by the way.

[epiqueras]

temporarily clone Gutenberg to another folder and set it up for master branch

Why not just pull master, format branch, and rebase?

I've updated the changelog and readme.

[ellatrix]

Why are all destructuring assignments collapsed? Is this by intention? In some cases, this results in really long lines which is less readable.

[jsnajdr]

Hi @ellatrix, can you post a simple example? To make sure we all talk about the same problem 🙂

[epiqueras]

Only the ones that fit under our line length rule.

[ellatrix]

@epiqueras @jsnajdr Here's an example.

[epiqueras]

Yeah, that fits under our line-length rule.

[ellatrix]

What's the line length rule? I don't see any previous mention of a line length rule. I thought we set it to maximum 80.

[epiqueras]

100 for WP.

[jsnajdr]

The current Prettier config sets the maximum to 100:

gutenberg/packages/scripts/config/.prettierrc.js

Line 4 in f63053c

printWidth: 100,

[aduth]

The standard is not strict, but recommended at 80-100:

Lines should usually be no longer than 80 characters, and should not exceed 100 (counting tabs as 4 spaces). This is a “soft” rule, but long lines generally indicate unreadable or disorganized code.

https://developer.wordpress.org/coding-standards/wordpress-coding-standards/javascript/#spacing

I personally use an editor.rulers VSCode setting to show a line at 80th column and adhere to it quite strictly in my own code. I've been struggling with this a bit more recently, at least with mixing more complex types into JSDoc (example of me relenting). For me, it's more of a mechanical, imperfect means to try to encourage myself flatten/simplify code. The difference between 80, 100, or maybe even 120 isn't going to make a huge difference in this. Unfortunately, at least for my purposes, I think it's one of Prettier's objectives to push this out of mind altogether (for better or worse).

I suppose we can still expect Prettier to collapse some of these destructurings even at 80 line length, but it is what it is.

[ellatrix]

It's a bit strange to me that we are now enforcing 80-100 lines on a single line. So currently it's wrong to destructure at the 80th column. The rule is strict.

I do think it's sometimes a bit annoying for docs (if types and variables are long), but for code it's almost never a difficulty to fit it within 80.

[ntwb]

Docs can be up to 100, though not sure if Prettier will take this into account:

https://developer.wordpress.org/coding-standards/inline-documentation-standards/javascript/#line-wrapping
DocBlock text should wrap to the next line after 80 characters of text. If the DocBlock itself is indented on the left 20 character positions, the wrap could occur at position 100, but should not extend beyond a total of 120 characters wide.

[epiqueras]

It won't: https://prettier.io/docs/en/options.html#print-width.

That's fine.

[ntwb]

Prettier doesn't actually touch comments, it leaves them as-is

See https://prettier.io/docs/en/rationale.html#comments

When it comes to the contents of comments, Prettier can’t do much really. Comments can contain everything from prose to commented out code and ASCII diagrams. Since they can contain anything, Prettier can’t know how to format or wrap them. So they are left as-is. The only exception to this are JSDoc-style comments (block comments where every line starts with a *), which Prettier can fix the indentation of.

[aduth]

Considering this will likely have introduced merge conflicts to a large majority of open pull request, is there a simple recommendation we can give to pull request authors who might not be familiar with the process for resolving those, or especially resolving them with Prettier now part of the pipeline?

[ItsJonQ]

@aduth That is a good point 😱

Maybe there's something we can add to one of the general README.md docs with rebase/merge instructions re: Prettier. Telling them what commands to run and in what sequence to resolve merge conflicts.

We can then link to that for current outstanding issues

[epiqueras]

@ellatrix The line lengths are not enforced. Prettier just tries to fit everything in as little lines as possible without breaking any of the rules, line length included. The consistency is worth getting used to. For merge conflicts I recommend: 1. Soft reset the PR. 2. Format only the changed files. 3. Commit. 4. Rebase onto master.

[noisysocks]

Loving that I never have to think about formatting again. Thanks all! 😍