Scripts: Switch to Prettier 2.5 and change formatting around spacing #37607
Conversation
gziolo
added
[Tool] WP Scripts
gziolo
force-pushed
the
update/prettier-2.5
branch
from
d9f6c4b to
04c3eeb
Compare
|
Size Change: -33 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
gziolo
force-pushed
the
update/prettier-2.5
branch
from
04c3eeb to
17e3992
Compare
gziolo
force-pushed
the
update/prettier-2.5
branch
from
17e3992 to
06889be
Compare
gziolo
changed the title
|
I'm also worried about how similar changes affect git Yesterday, I came across this conversation on Twitter. So maybe we should also consider adding Another commit we can consider for that list is 4857ad5. P.S. Article linked in a tweet - https://akrabat.com/ignoring-revisions-with-git-blame/ |
|
I have just posted an official proposal at https://make.wordpress.org/core/2022/01/05/proposal-changes-to-javascript-coding-standards-for-full-prettier-compatibility/. |
|
I'm all in for aligning to the community's standard. However, wouldn't it be more backward-compatible if we continue to follow the WordPress style guide but using So instead of running our own fork of prettier with Sure, it's gonna be slower, but I don't think we'd be running that command that often, and it can be automated with I agree that just changing the rules once and for all is a much straightforward solution though, but I also have the concern brought up by @Mamaduka in #37607 (comment) that git blaming would be a pain to deal with. |
Is the reason still relevant today? Seems like the conclusion in #4628 is to not use prettier which has obviously changed now. Might be worth noting that I've tried using |
I think this would be an interesting task to explore, wondering if we can create a dedicated ESLint configuration that only fixes "spaces between parens and brackets", and if running lint-staging with Prettier first and then this ESLint config would work.... It seems pretty hacky to me to be honest, and not very friendly to the wider developer community to have to implement it in this manner,not sure if |
As long people would use |
True, but that shouldn't matter either though. People should be able to use any styling during development, as long as the code they pushed is following WP standard, that's the reason we have
In theory, it should be possible, but I don't think we necessarily have to do this, it only has some performance benefits.
|
|
If the correct formating is only achieved using a CLI command like |
My imagination would be that Moreover, it should be possible to configure IDE integration to run |
Yeah, I guess it is possible. But each step we deviate from the standard workflow is usually an additional step users need to do to configure the tooling. In that case, they would need to figure out how to run that command instead of just installing a Prettier plugin. |
|
Thanks, I think it's worth exploring, if the workflows can be solved, without large amounts of pain, so that the Gutenberg Repo (Commit, Lint-Staged, GitHub Actions etc) and Developer IDE/Editor work (Prettier/ESLint save & format) If the above can be achieved, then Gutenberg no longer needs to use (or update & maintain) the Prettier fork and can use the canonical Prettier then that covers I think all of the main objections to the make/core proposal here: |
|
I did some quick testing, but it turns out there are unfortunately some rules that won't be able to stay intact. There's the // Original code that's really long and exceeds the `printWidth` setting
fn(
...
);
// Prettier: the width is just below the threshold after removing the spaces
fn(...);
// eslint adds back the spaces, but it won't wrap them into multiple lines anymore
fn( 123 );This creates some inconsistencies between the modified code and the original code. Even though the impact would be much lower than what this PR proposes, we still have to solve the same problem, just on a smaller scale.
We can provide documentation just like how we do currently. They don't "need" to though, I think it's an important point, the whole IDE setup should be optional. |
|
I also found PR where
Yes, that was one of the concerns discussed during the initial explorations of Prettier. ESLint operates after Prettier, and it doesn't respect the line width setting. The same issue likely applies to the Looking at the Anyway, you usually modify only a few JS files so that you won’t notice any issues. However, we use also Prettier in Markdown files. It’s getting more interesting there because Prettier is smart enough to detect JS snippets there and format them. However, ESLint knows nothing about Markdown, so we would diverge there, or we would have to ignore those snippets when using Prettier, so they don't get formatted without the ESLint step that fixes spaces. The most crucial part is optimizing the developer experience because many devs use IDE integrations. I know that the best part is that you just put a Prettier's config and everything magically works in Visual Studio Code when you save your code, and I suspect it's not that different experience in other editors listed at https://prettier.io/docs/en/editors.html. While Prettier + ESLint seems like a viable solution, I don't think it is clearly better than the current setup with the |
|
@gziolo Sorry for bothering you, but are there any news on the proposal/this PR? |
|
I'm re-sharing the comment from @ntwb that he left on WordPress Slack (link requires registration at https://make.wordpress.org/chat):
I'm closing this PR becaue of that. |
|
I opened a new issue in the Prettier repository that proposed again the option of allowing extra spaces around elements and arguments in JavaScript code: prettier/prettier#13107. |
Description
TLDR; I propose we switch to the original version of Prettier and update it to the most recent version 2.5. Consequently, we should also update the existing JavaScript Coding Standards around spacing to meet what Prettier can do.
Fixes #21872.
Fixes #37516.
Replaces and closes #37517.
We use an outdated fork of Prettier in the Gutenberg project because it didn't get any updates for more than a year:
wp-prettierprettierThe only motivation behind using a forked version of Prettier is its custom functionality overriding formatting behavior for spaces. It allows to align closer with the coding style convention used for PHP in the WordPress codebase as explained in JavaScript Coding Standards. Related rules:
It's worth reminding that this topic was discussed several times, beginning in 2017 when the initial exploration of using Prettier started with #2819. Throughout all that time, the Prettier maintainers didn't change their mind about adding support for spaces between parens (
()) and brackets ([]) despite many requests shared in prettier/prettier#1303 from users (including the WordPress community). That's why we decided to use the forked version developed by developers at Automattic.In the meantime, there were three minor releases of Prettier with a long list of new features in the last year:
The Gutenberg project no longer uses only JavaScript for development. Some parts of the codebase use TypeScript, which we agreed upon a few months back. It means we can't use improvements added for newer versions of the TypeScript language and potentially for future additions to the JavaScript language. A deprecation was also added to one of the config options we use in the shared Prettier config that the community uses. It turns out that it prevents using
@wordpress/prettier-configwith the most recent version of Prettier in 3rd party projects, as explained in #37516. Finally, we had multiple reports in #21872 from the community members using@wordpress/scriptsor@wordpress/eslint-pluginthat the forked version of Prettier doesn't install correctly in their projects. The good news is that switching to the original Prettier will resolve all of the above!JavaScript Coding Standard changes necessary
- Use spaces liberally throughout your code. “When in doubt, space it out.”There are more code examples that we would have to update in the document, but they are very similar to those shared above.
How has this been tested?
I also ensured that there were no changes in the branch when running:
Types of changes
Breaking change (fix or feature that would cause existing functionality not to work as expected).
TODO tasks
Checklist:
*.native.jsfiles for terms that need renaming or removal).