-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Build Tools: Try prettier
+ eslint --fix
#4628
Conversation
A testing workflow of this PR could be something like: • Create a new branch of Finally, testing the pre-commit hook can be achieved by making some changes to a JS file or two and add them via p.s. I suggested the method above to avoid accidental pushing of changes back up to this branch 😉 |
Reading and sanity checking myself, this PR doesn't actually use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR @ntwb — it's awesome that you're trying to add Prettier in the mix. Here're some of my thoughts about this.
— This is a draft of what I think and where I am stuck while trying to implement Prettier in create-guten-block
.
@@ -132,12 +135,23 @@ | |||
}, | |||
"verbose": true | |||
}, | |||
"lint-staged": { | |||
"*.js": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only .js
files. We should be using Prettier for .css
, .less
, .scss
, or .json
files as well. Since, ESLint can't.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're only discussed using Prettier for JavaScript files at this stage, I plan on proposing we also use it for SCSS & CSS files but there's some work to do here to bring stylelint-config-wordpress up to date, in essence, let's walk before we run 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, let’s tackle JS only in this PR. I would also like us to do the rest lint & format work inside ‘@wordpress/packages’ first and consume through ‘@wordpress/scripts’.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, well that makes sense.
"lint-staged": { | ||
"*.js": [ | ||
"prettier --write", | ||
"eslint --fix", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no use to run eslint --fix
command with the current .eslintrc.json
file.
—Why?
❌ Basically, this is what's happening here
- Prettier fixes and rewrites the files
- ESLint again fixes them with many overlapping formatting rules (potentially doing again what it was doing without prettier)
✅ What should happen here?
- Prettier fixes and rewrites the files
- ESLint fixes what Prettier can't
- ESLint leaves formatting of all the overlapping rules to prettier using something like
prettier-eslint
but that's not it, we also needeslint-plugin-prettier
to do that and to format only the conflicting rules, we need to forkeslint-config-prettier
or use before and add more rules custom rules after that where Prettier fails WP coding standards.
This is a draft of what I think and where I am stuck while trying to implement Prettier in create-guten-block
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And what is wrong with that? It's just CPU cycles 😉
It's pretty quick as it is only pretty printing and linting staged files
I honestly don't think prettier-eslint
is all that helpful, it requires a formatter that can pretty print in the Editor or IDE, a formatter plugin is not available for some of the common IDEs and Editors out there, the configuration of each is also different, this adds another burden on developers to configure another tool (See also this @JJJ Tweet from earlier today)
The eslint-plugin-prettier
from what I gather does what it says in the description of the plugin "Runs Prettier as an ESLint rule and reports differences as individual ESLint issues.", now users see even more ESLint warnings coming from Prettier via the ESLint command.
If we can hide as many errors from developers we can the better for all concerned.
So, to that end, I don't see an issue having Prettier then ESLint fixing and writing the files, It's pretty quick as it is only performing this on staged files
There is no extra tooling burden added to developers with this approach, and it's probably not ideal but it's just robots processing code, actually, just a few CPU cycles 😏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a good idea to add both eslint-plugin-prettier and eslint-config-prettier and see if it integrates well. That way you would have to run ‘eslint —fix’ only as a precommit hook. See this article: https://medium.com/@netczuk/your-last-eslint-config-9e35bace2f99.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with eslint-config-prettier
is it turns off rules we want to keep, e.g space-in-parens.
The plugin eslint-plugin-prettier
uses Prettier to pretty print so why not just let Prettier do this and not add another dependency? I've honestly struggled to see any benefit at all of this plugin to be honest, all it does it shows extra ESLint errors for issues that Prettier is required to fix, to me, it seems to just add more noise /shrug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And what is wrong with that? It's just CPU cycles 😉
I think it's more than just that. When an IDE picks up conflicting tools then you end up in a loop of one tool fixing an issue and creating that issue for another tool and vice versa.
The eslint-plugin-prettier from what I gather does what it says in the description of the plugin "Runs Prettier as an ESLint rule and reports differences as individual ESLint issues.",
No actually, it does not. It does that in a combination with eslint-config-prettier
without that, it's a loop of one tool fixing and breaking things for another.
There is no extra tooling burden added to developers with this approach, and it's probably not ideal but it's just robots processing code, actually, just a few CPU cycles
Again the problem here is the same. Also, I kind of dislike linting + formatting on staged files. That way something wrong can end up in the code base and all you have to say about that is I didn't write it that way — but the tools ended up linting it all at the end.
Also since the codebase is not just getting started — this can pose more problems. Having it run progressively via editors is my preferred way of doing things. But that's just me.
Right now what worries me here is the one tool's fixes are another tool's problems!
@ntwb @gziolo I am actually trying to make this doable in a single package. I'd love to do it with you folks and then we can add it here as a single dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My "It's just CPU cycles" has nothing to do with editors or IDEs, its run via Git as a commit hook
FYI: This PR isn't just about Gutenberg, it's also about the feasibility of adding it to WordPress core, for the most part this will be in part achievable by us sharing this tooling via the @wordpress/packages
library that @gziolo has already mentioned, our goal here is to have it available to share with plugin and theme devs, so once we've got it sorted, you'll be able to also use it in your package.
p.s. See my other comment on why eslint plugin/config-prettier are not suitable for us
Here's an ESLint summary of what I've proposed above: After running ./node_modules/.bin/eslint -f node_modules/eslint-formatter-summary . -- --sort-by errors --desc
✨ Summary of failing ESLint rules
errors 26749 warnings 0 rule: space-in-parens
errors 4354 warnings 0 rule: react/jsx-curly-spacing
errors 1256 warnings 0 rule: array-bracket-spacing
errors 866 warnings 0 rule: computed-property-spacing
errors 603 warnings 0 rule: space-unary-ops
errors 316 warnings 0 rule: template-curly-spacing
errors 158 warnings 0 rule: operator-linebreak
errors 53 warnings 0 rule: indent
errors 35 warnings 0 rule: quotes
errors 24 warnings 0 rule: no-mixed-operators
errors 3 warnings 0 rule: no-unused-vars
errors 2 warnings 0 rule: jsx-a11y/anchor-is-valid
errors 1 warnings 0 rule: no-alert
🔥 34420 problems in total (34420 errors, 0 warnings) Following that, running ./node_modules/.bin/eslint -f node_modules/eslint-formatter-summary . -- --sort-by errors --desc
✨ Summary of failing ESLint rules
errors 24 warnings 0 rule: no-mixed-operators
errors 3 warnings 0 rule: no-unused-vars
errors 2 warnings 0 rule: jsx-a11y/anchor-is-valid
errors 1 warnings 0 rule: no-alert
🔥 30 problems in total (30 errors, 0 warnings)
``
30 warnings are significantly less than I would have guessed we'd have seen... |
Running prettier results in +34,000 warnings? 😃 |
Running eslint fix results in +34,000 fixes! 😏 Here's a diff of the results of Prettier followed by ESLint fixes: • https://gist.github.com/ntwb/74b3fa63a87f1d97bde7edf84c5491f0 It's only a ~24,000 diff file, I think it looks pretty 😄 |
If those prettier pair of preset and plugin don’t change anything I think we can skip them, but still I would at least look at what they do behind the scenes. Last time I checked eslint —fix on the master didn’t update anything so we should be good, but I would like us to double check. |
I've not looked behind the scenes of
Adding the /Users/netweb/dev/wordpress/gutenberg/webpack.config.js
...
...
...
4:25 error Replace `·'webpack'·` with `'webpack'` prettier/prettier
5:35 error Replace `·'extract-text-webpack-plugin'·` with `'extract-text-webpack-plugin'` prettier/prettier
6:34 error Replace `·'webpack-rtl-plugin'·` with `'webpack-rtl-plugin'` prettier/prettier
9:56 error Delete `·` prettier/prettier
11:2 error Delete `·` prettier/prettier
14:51 error Delete `·` prettier/prettier
16:2 error Delete `·` prettier/prettier
19:47 error Delete `·` prettier/prettier
21:2 error Delete `·` prettier/prettier
30:15 error Replace `⏎↹↹↹↹↹require(·'autoprefixer'·),⏎↹↹↹↹` with `require('autoprefixer')` prettier/prettier
38:20 error Replace `·'editor/assets/stylesheets'·` with `'editor/assets/stylesheets'` prettier/prettier
39:10 error Replace `·` with `⏎↹↹↹↹↹` prettier/prettier
40:17 error Replace `·'production'·===·process.env.NODE_ENV·?⏎↹↹↹↹↹` with `⏎↹↹↹↹↹'production'·===·process.env.NODE_ENV·?·` prettier/prettier
58:23 error Replace `⏎↹'hooks',⏎` with `'hooks'` prettier/prettier
71:2 error Replace `·...entryPointNames,·...packageNames·].forEach(·` with `...entryPointNames,·...packageNames].forEach(` prettier/prettier
72:12 error Replace `·`@wordpress/${·name·}`·` with ``@wordpress/${name}`` prettier/prettier
73:10 error Replace `·'wp',·name·` with `'wp',·name` prettier/prettier
75:2 error Delete `·` prettier/prettier
79:26 error Replace `·(·memo,·entryPointName·` with `(memo,·entryPointName` prettier/prettier
80:9 error Replace `·entryPointName·]·=·`./${·entryPointName·` with `entryPointName]·=·`./${entryPointName` prettier/prettier
82:8 error Delete `·` prettier/prettier
83:23 error Replace `·(·memo,·packageName·` with `(memo,·packageName` prettier/prettier
84:9 error Replace `·packageName·]·=·`./node_modules/@wordpress/${·packageName·` with `packageName]·=·`./node_modules/@wordpress/${packageName` prettier/prettier
86:8 error Delete `·` prettier/prettier
91:13 error Replace `·'wp',·'[name]'·` with `'wp',·'[name]'` prettier/prettier
96:13 error Replace `⏎↹↹↹__dirname,⏎↹↹↹'node_modules',⏎↹↹` with `__dirname,·'node_modules'` prettier/prettier
114:15 error Replace `⏎↹↹↹↹↹/blocks/,⏎↹↹↹↹` with `/blocks/` prettier/prettier
117:34 error Replace `·extractConfig·` with `extractConfig` prettier/prettier
121:15 error Replace `⏎↹↹↹↹↹/blocks/,⏎↹↹↹↹` with `/blocks/` prettier/prettier
124:38 error Replace `·extractConfig·` with `extractConfig` prettier/prettier
128:15 error Replace `⏎↹↹↹↹↹/blocks/,⏎↹↹↹↹` with `/blocks/` prettier/prettier
131:43 error Replace `·extractConfig·` with `extractConfig` prettier/prettier
136:28 error Delete `·` prettier/prettier
137:43 error Replace `·process.env.NODE_ENV·||·'development'·` with `⏎↹↹↹↹process.env.NODE_ENV·||·'development'⏎↹↹↹` prettier/prettier
138:4 error Delete `·` prettier/prettier
143:24 error Delete `·` prettier/prettier
146:4 error Delete `·` prettier/prettier
147:35 error Delete `·` prettier/prettier
150:4 error Delete `·` prettier/prettier
157:9 error Replace `·process.env.NODE_ENV·` with `process.env.NODE_ENV` prettier/prettier
159:23 error Replace `·new·webpack.optimize.UglifyJsPlugin()·` with `new·webpack.optimize.UglifyJsPlugin()` prettier/prettier
✖ 17497 problems (17497 errors, 0 warnings)
17497 errors, 0 warnings potentially fixable with the `--fix` option. It simply displays errors for what Prettier would fix, the ESLint rule could be changed to Grrrr: Fat fingers, I closed the PR 😢 |
prettier.config.js
Outdated
@@ -0,0 +1,11 @@ | |||
module.exports = { | |||
arrowParens: 'avoid', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking around our codebase, I see ( x ) => foo( x )
more than I see x => foo( x )
. Should we set this to 'always'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I may have peeked at Calypso in the makings of this 😏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in e258034
"scripts": { | ||
"prebuild": "check-node-version --package", | ||
"build": "cross-env BABEL_ENV=default NODE_ENV=production webpack", | ||
"gettext-strings": "cross-env BABEL_ENV=gettext webpack", | ||
"lint": "eslint .", | ||
"lint:fix": "eslint . --fix", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If nothing else, it would be good to get this command into master 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in #4712
Following commit e258034 there are 1,305 extra Here's a follow up diff https://gist.github.com/ntwb/36420b0e477996ed4ac0a1ac23b5a36f |
There are 223 instances of this type of change by Prettier here: case 'admin-appearance':
- path = 'M14.48 11.06L7.41 3.99l1.5-1.5c.5-.56 2.3-.47 3.51.32 1.21.8 1.43 1.28 2.91 2.1 1.18.64 2.45 1.26 4.45.85zm-.71.71L6.7 4.7 4.93 6.47c-.39.39-.39 1.02 0 1.41l1.06 1.06c.39.39.39 1.03 0 1.42-.6.6-1.43 1.11-2.21 1.69-.35.26-.7.53-1.01.84C1.43 14.23.4 16.08 1.4 17.07c.99 1 2.84-.03 4.18-1.36.31-.31.58-.66.85-1.02.57-.78 1.08-1.61 1.69-2.21.39-.39 1.02-.39 1.41 0l1.06 1.06c.39.39 1.02.39 1.41 0z';
+ path =
+ 'M14.48 11.06L7.41 3.99l1.5-1.5c.5-.56 2.3-.47 3.51.32 1.21.8 1.43 1.28 2.91 2.1 1.18.64 2.45 1.26 4.45.85zm-.71.71L6.7 4.7 4.93 6.47c-.39.39-.39 1.02 0 1.41l1.06 1.06c.39.39.39 1.03 0 1.42-.6.6-1.43 1.11-2.21 1.69-.35.26-.7.53-1.01.84C1.43 14.23.4 16.08 1.4 17.07c.99 1 2.84-.03 4.18-1.36.31-.31.58-.66.85-1.02.57-.78 1.08-1.61 1.69-2.21.39-.39 1.02-.39 1.41 0l1.06 1.06c.39.39 1.02.39 1.41 0z';
break; These type of line-length changes are not ideal, but I guess that's what we give up if we start using Prettier, not easily documenting this in the coding standards either. These instances could be ignored using |
https://gist.github.com/ntwb/36420b0e477996ed4ac0a1ac23b5a36f#file-4628-2nd-pass-test-diff-L11163 diff --git components/dropdown/index.js components/dropdown/index.js
index 449e7afc..7bfd3d58 100644
--- components/dropdown/index.js
+++ components/dropdown/index.js
@@ -75,9 +75,9 @@ class Dropdown extends Component {
return (
<div className={ className } ref={ this.bindContainer }>
{ /**
- * This seemingly redundant wrapper node avoids root return
- * element styling impacting popover positioning.
- */ }
+ * This seemingly redundant wrapper node avoids root return
+ * element styling impacting popover positioning.
+ */ }
<div>
{ renderToggle( args ) }
<Popover Prettier has changed that inline doc from: { /**
* This seemingly redundant wrapper node avoids root return
* element styling impacting popover positioning.
*/ } To: { /**
* This seemingly redundant wrapper node avoids root return
* element styling impacting popover positioning.
*/ } |
https://gist.github.com/ntwb/36420b0e477996ed4ac0a1ac23b5a36f#file-4628-2nd-pass-test-diff-L1825 @@ -333,8 +329,8 @@ describe( 'block parser', () => {
const parsed = parse(
'<!-- wp:core/test-block {"smoked":"yes","url":"http://google.com","chicken":"ribs & \'wings\'"} -->' +
- 'Brisket' +
- '<!-- /wp:core/test-block -->'
+ 'Brisket' +
+ '<!-- /wp:core/test-block -->'
);
expect( parsed ).toHaveLength( 1 ); Prettier has changed this from:: const parsed = parse(
'<!-- wp:core/test-block {"smoked":"yes","url":"http://google.com","chicken":"ribs & \'wings\'"} -->' +
'Brisket' +
'<!-- /wp:core/test-block -->'
); To: const parsed = parse(
'<!-- wp:core/test-block {"smoked":"yes","url":"http://google.com","chicken":"ribs & \'wings\'"} -->' +
'Brisket' +
'<!-- /wp:core/test-block -->'
); |
I hadn't noticed that one 😞
This will be due to
|
@ntwb |
80 for the line width is the default setting and something that JS projects use, see React or Babel. |
I think that proposed Prettier’s config is as close as we can get to WordPress coding styles taking into account what they offer. If you are really concern about other stuff we can always build our own eslint rules accompanied with —fix command. However I feel like it’s better to simply update the coding styles because we are very close to what we want to have. I don’t have time this week to play with this code but I’d be more than happy to see it merged 👍 |
There's another case of this here: on Lines 3689/3690: - { top: 0, left: -( ( blockPadding * 2 ) + blockMoverMargin ) };
+ { top: 0, left: -( blockPadding * 2 + blockMoverMargin ) }; This change causes 2 of the 30 ESLint errors now reported:
The same is true for another 6 instances I looked at, testing if this is caused by Prettier or when ESLint fixing.... It's a Prettier thing: - const toolbarOffset = this.props.inlineToolbar ?
- { top: 10, left: 0 } :
- { top: 0, left: -( ( blockPadding * 2 ) + blockMoverMargin ) };
+ const toolbarOffset = this.props.inlineToolbar
+ ? { top: 10, left: 0 }
+ : { top: 0, left: -(blockPadding * 2 + blockMoverMargin) }; After ESLint fixing: - { top: 0, left: -( ( blockPadding * 2 ) + blockMoverMargin ) };
+ { top: 0, left: -( blockPadding * 2 + blockMoverMargin ) }; I'm now just going to assume that all 24 of the |
@ntwb That's exactly what I was talking about earlier. We have to move the formatting to prettier except for the things that ESLint needs to handle (where Prettier fails). That's why I was not able to add it to create-guten-block. We need to build a custom configuration for both Prettier and ESLint to manage this. 💯 |
Prettier and WordPress are both pretty opinionated, though there's only a couple of instances where those opinions really opposed each other, thankfully ESLint's To that end examing whats left after WordPress has applied its ESLint fixes to the changes made by Prettier is relatively minor, primarily these changes relate to line length, where Prettier refactors the code to not exceed For the remaining instances of stylistic formatting that Prettier made we can look to add ESLint rules to detect these style inconsistencies and update our coding standards to match. Hopefully we can contribute ESLint rules and fixes upstream as much as possible for the benefit of all ESLint users. I don't think Prettier is the right solution for WordPress Core, we'll continue to enhance and iterate our JavaScript Coding Standards with ESLint. Thanks to everyone who has tested and explored the world of Prettier with us here, I've had a blast testing this, closing this PR now, thanks again everyone 😄 |
If any contributors want to use Prettier to format the code as they write and/or save on the fly in their IDE/Editor of choice, sure go for it, what we won't be doing is running Prettier across the entire JS codebase per the reasons stated above, as long as the code passes our ESLint lint tests and meets the WordPress JavaScript Coding Standards how or what tools you choose to write your code with before submitting a pull request is up to you. I've added the npm script @gziolo I've closed #2819 citing my comment above @noisysocks Is there anything else needed to help you continue to run Prettier? Ignores in |
I don't need anything else. Manually running prettier (via VS Code) on one file at a time has been working pretty well for me. |
prettier-eslint
prettier
+ eslint --fix
Description
See #2819
How Has This Been Tested?
Running various combinations of the following scripts:
•
npm run prettier:check
to list JS files that Prettier can fix•
npm run prettier:fix
to fix JS files using Prettier defined configuration inprettier.config.js
•
npm run lint
To run ESLint using the current ESLint.eslintrc.js
configuration•
npm run lint:fix
to fix JS files using EESLint defined configuration inprettier.config.js
Types of changes
To explore using Prettier and ESLint side by side, Prettier will print the code using as many available options from the WP Coding Standards as possible, ESLint will then fix any issues not available as a Prettier option.
Some considerations I hope to have covered in this PR:
• Contributors can continue to code in any Editor/IDE of their choice
• If an ESLint plugin is available for the Editor/IDE it can be used with the current
.eslintrc.js
• If the ESLint plugin used above supports autofixing on file save via a
--fix
option this can be used• During commit a pre-commit
lint-staged
hook is executed, this script runsprettier --write
followed byeslint --fix
only on Git staged files and not the entire codebaseToDo:
Run
npm run prettier:fix
and examine the changes and iterateprettier.config.js
as needed.Run
npm run lint:fix
after having runnpm run prettier:fix
and examine the file changes.Comparing the results from the above against the WordPress JavaScript Coding Standards
Discuss any changes to the coding standards that may be warranted
Checklist: