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 Prettier formatting script #18048

Open
wants to merge 8 commits into
base: master
from
Open

Add Prettier formatting script #18048

wants to merge 8 commits into from

Conversation

@jsnajdr
Copy link
Contributor

jsnajdr commented Oct 21, 2019

This PR adds support for wp-prettier, a Prettier fork that follows the WordPress code conventions and inserts extra spaces inside parens:

function fun( param ) {
  foo( [ 1, 2 ], ( val ) => {
    return `key-${ val }`;
  } );
}

I've been maintaining the fork for 2+ years, from version 1.5 to the latest 1.18.2, and it's been super-useful in Automattic projects like Calypso or Jetpack.

Add a format-js script to @wordpress/scripts:
There is a new script that can be used to format a source directory like this:

wp-scripts format-js packages/compose

The arguments are a list of files and directories to be formatted (globs are possible, too), and it will format all matching JS files, while respecting the ignores in the default .eslintignore.

Remove conflicting ESLint rules:
Some ESLint rules that guard formatting are known to conflict with Prettier (difference in opinions about what's the "right" format), so I'm disabling these that check for things that are automatically handled by Prettier. There's also an eslint-config-prettier ESLint plugin for that.

Format packages/compose to get a taste of Prettier:
This PR also includes formatting of one small module to get a taste of how the formatting changes look like.

TODO: incremental adoption
One proven way how to adopt Prettier incrementally in a large codebase is this one:

  1. Whenever a file is formatted with Prettier, add a /** @format */ pragma at the beginning. That tells the automated tooling to try keep it formatted.
  2. In a pre-commit hook, check for @format pragma in the modified file and reformat it before committing if the pragma is present. Can be done with lint-staged tool that we already use.

The tooling for that is NOT in this PR. Can be added later if there's an interest in Prettier in the first place.

hasProjectFile( '.prettierrc.yaml' ) ||
hasProjectFile( '.prettierrc.yml' ) ||
hasProjectFile( '.prettierrc.js' ) ||
hasProjectFile( '.prettierrc.config.js' ) ||

This comment has been minimized.

Copy link
@ntwb

ntwb Oct 21, 2019

Member
Suggested change
hasProjectFile( '.prettierrc.config.js' ) ||
hasProjectFile( 'prettierrc.config.js' ) ||

No leading . period, Prettier uses cosmiconfig, see https://prettier.io/docs/en/configuration.html

"eslint": "^6.1.0",
"jest": "^24.7.1",
"jest-puppeteer": "^4.3.0",
"js-yaml": "^3.13.1",
"lodash": "^4.17.15",
"minimist": "^1.2.0",
"npm-package-json-lint": "^3.6.0",
"prettier": "npm:wp-prettier@^1.18.2",

This comment has been minimized.

Copy link
@ntwb

ntwb Oct 21, 2019

Member

What functionality does the npm: prefix here perform if you don't mind, I've not seen this?

This comment has been minimized.

Copy link
@swissspidy

swissspidy Oct 22, 2019

Member

It aliases prettier to wp-prettier, so it will actually use the wp-prettier package under the hood. This was introduced in npm 6.9.

This comment has been minimized.

Copy link
@ntwb

ntwb Oct 22, 2019

Member

Neat, thanks, this is pretty cool

This comment has been minimized.

Copy link
@gziolo

gziolo Oct 22, 2019

Member

We would need to enforce that check-engines requires npm 6.9 or higher by default.

This comment has been minimized.

Copy link
@swissspidy

swissspidy Oct 22, 2019

Member

Even cooler would be not having to maintain such a fork, but progress is slow on that front: prettier/prettier#5919

This comment has been minimized.

Copy link
@jsnajdr

jsnajdr Oct 22, 2019

Author Contributor

Note that even if prettier/prettier#5919 was fixed and we could ship wp-prettier as a plugin, it wouldn't change much. We'd still need to maintain a fork of the JS formatter, and then ship the forked formatter as a plugin that replaces the original JS formatter. The code and the work is still the same, just shipped in different packaging.

I could fix prettier/prettier#5919 myself if needed, but with package aliasing support shipping both in NPM and Yarn, the aliasing method works equally well.

This comment has been minimized.

Copy link
@jsnajdr

jsnajdr Oct 22, 2019

Author Contributor

This was introduced in npm 6.9.

Yes, and it's the main reason why I waited so long before proposing to use the fork. Before package aliases, it could be distributed only as a tarball URL or a GitHub tag ref, which is kind of ugly and doesn't support versioning well. Now, distributing forks of things is much cleaner and easier.

@@ -0,0 +1,12 @@
module.exports = {
useTabs: true,
tabWidth: 2,

This comment has been minimized.

Copy link
@ntwb

ntwb Oct 21, 2019

Member

Should this be defined? Some devs like 4 spaces, some 2, even 8.

Leaving this as a developer preference for them to set in their Editor/IDE is in line with how WordPress Core uses .editorconfig by not defining a tab width

This comment has been minimized.

Copy link
@jsnajdr

jsnajdr Oct 22, 2019

Author Contributor

useTabs means that the formatted source will contain tabs and editors can display them freely according to their configuration.

tabWidth is needed only to calculate the line length. If the maximum line length is 80, and the code is indented with tabs, what does the line length number even mean? Nothing. We need to define tabWidth to give the tab a dimension.

This comment has been minimized.

Copy link
@ntwb

ntwb Oct 22, 2019

Member

Thanks for the explanation 👍🏼

@ntwb

This comment has been minimized.

Copy link
Member

ntwb commented Oct 21, 2019

Thanks for the PR @jsnajdr I've added a couple of comments for starters...

What I would like to see though is how Prettier has, or if it has since we looked at it in #4628, our conclusion at the time was this (copying here for ease ref) #4628 (comment):

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 --fix was able to bring a satisfactory compromise.

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 x characters in length, it does a pretty good job of that for the most part. As noted in the comments above sometimes it's not ideal where and when it does this it makes for poorly formatted code.

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 😄

I would love to see how Prettier fares today with some of the issues we raised as concerns:

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Oct 22, 2019

Noting that this PR addresses #2819 which was closed a long time ago due to the issues listed in the previous comment by @ntwb.

@jsnajdr

This comment has been minimized.

Copy link
Contributor Author

jsnajdr commented Oct 22, 2019

I would love to see how Prettier fares today with some of the issues we raised as concerns:

To be honest, the formatting hasn't changed much 🙂 Looking at the concerns that had been raised, I see two where a fix could be proposed upstream:

Botching up alignment of docblock comments inside JSX

<div>
  { /**
   * i'm a docblock
   */ }
</div>;

This is clearly a Prettier bug, as it happens only when the JSX expression container is otherwise empty. Adding anything to it fixes the comment formatting.

Unpleasant breaking of long curry chains with arrowParens=always

const f = ( a ) => ( b ) => (
  c
) => find( a, b, c );

The arrowParens=avoid version of this code doesn't break the line, even if the curry chain is longer than the maximum line length:

const f = a => b => c => // could continue ad infinitum and never breaks
  find( a, b, c );

I think preventing a break for arrow function with a single simple argument is a reasonable change that shouldn't meet much opposition.


But formatting details aside, I think the main reason why this new attempt at Prettier is different is that it removes the need for the eslint --fix step. wp-prettier can insert all the whitespace inside parens correctly, as the WordPress convention requires, and doesn't need any other tool to fix it. There is a new parenSpacing option for that.

Everything else, as I see it, are just subtle differences in opinion about how things should be formatted, they are not specific to the "WordPress culture", if you know what I mean, and don't have a rational resolution that everyone can agree on.

If something seems obviously wrong, as the examples above, we can propose patches upstream. The only WordPress-specific difference seems to be the paren spacing.

@ntwb

This comment has been minimized.

Copy link
Member

ntwb commented Oct 22, 2019

Thanks for the detailed reply @jsnajdr 💯

I think it would great to see how this looks on the entire code base, a gist, a fork maybe, then the Gutenberg and WordPress teams can take a closer look...

I'm thinking once the above is available we can document what changes would be required and acceptable to update the WordPress Coding Standards with, and then create a P2 on make.w.org/core for further feedback

@jsnajdr

This comment has been minimized.

Copy link
Contributor Author

jsnajdr commented Oct 22, 2019

I think it would great to see how this looks on the entire code base, a gist, a fork maybe, then the Gutenberg and WordPress teams can take a closer look...

I can reformat the whole codebase and push the diff to this branch. I originally decided to avoid that in order to keep the PR small.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Oct 22, 2019

I can reformat the whole codebase and push the diff to this branch. I originally decided to avoid that in order to keep the PR small.

It would be fun to see a branch with all files formatted :)

@jsnajdr jsnajdr force-pushed the add/prettier branch from 4c82806 to d2efdd4 Nov 22, 2019
@jsnajdr

This comment has been minimized.

Copy link
Contributor Author

jsnajdr commented Nov 22, 2019

Pushed several changes:

  • don't remove the formatting rules from the ESLint config. Rather disable them by including eslint-config-prettier in the recommended config. They are still available as recommended-with-formatting config for these who want to opt out (requested by @aduth) among others.
  • update the configuration of the operator-linebreak rule to require ? and : operators (ternary) to be after linebreak rather than before. Unlike binary operators like +. Done by using the default supplied by ESLint core and removing our override.

The result is that by default, recommended ESLint configs provided by the @wordpress/* toolset don't check formatting and a Prettier script and config are provided. But projects can opt-out by not using the format-js script and by using the recommended-with-formatting ESLint config instead. How does that sound?

@aduth aduth mentioned this pull request Dec 11, 2019
4 of 5 tasks complete
@ntwb

This comment has been minimized.

Copy link
Member

ntwb commented Dec 14, 2019

A thought just crossed my mind whilst working on the VS Code stylelint extension, I noticed these VS Code settings for Prettier:

  "prettier.arrowParens": "always",
  "prettier.bracketSpacing": true,
  "prettier.jsxBracketSameLine": false,
  "prettier.printWidth": 100,
  "prettier.semi": true,
  "prettier.singleQuote": true,
  "prettier.tabWidth": 2,
  "prettier.trailingComma": "es5",
  "prettier.useTabs": true,

How would would the parenSpacing: true, setting work with the VS Code extension?

I suspect the VS Code Prettier extension will ignore the "prettier.parenSpacing": true, setting if it was added...

Will need to do a bit more research on this to determine how the extension detects configs, Prettier uses cosmiconfig, though I'm unsure what configuration would take precedence, the config from the repo, or the VS Code extension setting.

@noisysocks I know you've been using VS Code and Prettier forever with Gutenberg, have you hit upon this issue or have some thoughts they would be greatly appreciated...

@jsnajdr

This comment has been minimized.

Copy link
Contributor Author

jsnajdr commented Dec 16, 2019

@ntwb I don't know how the VSCode Prettier integration works, but I'm familiar with the Atom integration, which works as follows:

Configuration: The IDE integration tries to find a Prettier config file (.prettierrc or one of the many alternative names, including a package.json field) and uses that when formatting files inside the project. The configuration in the IDE settings is only a fallback, default that is used if no other config files is found.

Prettier binary: Similarly, the Atom integration tries to find a Prettier binary in the project's node_modules folder and will use it if found. Only when it's not found will it use the Prettier that is bundled and shipped together with the plugin. If Prettier is in node_modules, it can be ensured that all contributors use the same version. Also, in our case, where we use a fork, the difference from the IDE-bundled Prettier is much bigger.

For both of the above to work correctly, the Prettier config needs to be in the project's top-level folder and the Prettier binary needs to be in the top-level node_modules folder. If it's in any other location, e.g., in a packages/scripts subfolder, IDEs and other tools won't find it reliably.

If the project-wide config and binary are set up correctly, the defaults of the IDE integration should not matter.

This is true for the Atom integration I'm familiar with, but I'd expect that other editor integration work exactly the same way.

@jsnajdr jsnajdr force-pushed the add/prettier branch from d2efdd4 to d077c5e Dec 16, 2019
@jsnajdr

This comment has been minimized.

Copy link
Contributor Author

jsnajdr commented Dec 16, 2019

Latest updates for the format-js script in wp-scripts:

The script requires that the Prettier fork is installed in the project root and checks that it's indeed the wp-prettier fork and not the official prettier. It will report an error if Prettier is not found or unexpected version.

Prettier is no longer a dependency of the wp-scripts package itself. For IDE and editor integrations to work reliably, it always needs to be installed in the project's root node_modules. Not somewhere in node_modules/@wordpress/scripts/node_modules/prettier -- the editor integrations won't find it there.

For the same reason, we require that a project has a top-level .prettierrc.js file. The format-js script doesn't default to the one that ships inside packages/wp-scripts/config. Again, editor integrations wouldn't find it there.

A project can add an one-liner .prettierrc.js config file and everything will be fine:

module.exports = require( '@wordpress/scripts/config/.prettierrc.js' );
@ntwb

This comment has been minimized.

Copy link
Member

ntwb commented Dec 16, 2019

Thanks for the detailed report @jsnajdr, I'll need to test VS Code but I would suspect it would perform in a similar fashion.

Thanks for updating the PR to account for this, and the docs tweak #19074 (comment)

We will need to update the @wordpress/scripts later to also make sure this is well documented.

@jsnajdr jsnajdr force-pushed the add/prettier branch from d077c5e to 9b8682d Jan 14, 2020
@jsnajdr

This comment has been minimized.

Copy link
Contributor Author

jsnajdr commented Jan 14, 2020

There's an e2e failure in this PR (even after a rebase) that seems 100% unrelated to Prettier:

FAIL packages/e2e-tests/specs/editor/blocks/buttons.test.js (7.133s)
  Buttons
    ✓ has focus on button content (3195ms)
    ✕ can jump to the link editor using the keyboard shortcut (3342ms)
  ● Buttons › can jump to the link editor using the keyboard shortcut
    expect(received).toMatchSnapshot()
    Snapshot name: `Buttons can jump to the link editor using the keyboard shortcut 1`
    - Snapshot
    + Received
      "<!-- wp:buttons -->
      <div class="wp-block-buttons"><!-- wp:button -->
    - <div class="wp-block-button"><a class="wp-block-button__link">WordPress</a></div>
    + <div class="wp-block-button"><a class="wp-block-button__link" href="https://wwww.wordpress.o" title="https://wwww.wordpress.o">WordPress</a></div>
      <!-- /wp:button --></div>
      <!-- /wp:buttons -->"
      28 | 		await page.keyboard.press( 'Enter' );
      29 | 
    > 30 | 		expect( await getEditedPostContent() ).toMatchSnapshot();
         | 		                                       ^
      31 | 	} );
      32 | } );
      33 | 
      at Object.toMatchSnapshot (specs/editor/blocks/buttons.test.js:30:42)
          at runMicrotasks (<anonymous>)
 › 1 snapshot failed.

@jorgefilipecosta any idea what's wrong here? Also cc @adamsilverstein who asked about the failure in #core-js Slack.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 14, 2020

@jsnajdr See also #19490. It's a known intermittent failure. I'll restart the build. I'm hopeful we can get the underlying issue resolved soon.

@jsnajdr

This comment has been minimized.

Copy link
Contributor Author

jsnajdr commented Jan 14, 2020

It's a known intermittent failure. I'll restart the build.

Thanks! I got the failure two times in a row and that's what confused me 🙂

@jsnajdr

This comment has been minimized.

Copy link
Contributor Author

jsnajdr commented Jan 15, 2020

@gziolo @aduth this PR still needs review. I'm not aware of any further issues that would need to be resolved. Can you help me with getting it shipped?

Copy link
Member

gziolo left a comment

There are two tasks left and we are good to go:

  • we need to document changes introduced in CHANGELOG files for the ESLint plugin and @wordpress/scripts package
  • we need to update related docs in both packages update
    • there needs to be a section on how to run format command with wp-scripts, how to integrate with IDE
    • for the ESLint plugin we need to add a note about recommended-with-formatting ruleset which gets exposed
hasPackageProp( 'prettier' );

if ( ! hasProjectPrettierConfig ) {
return {

This comment has been minimized.

Copy link
@gziolo

gziolo Jan 15, 2020

Member

It's less pressing for the start but I would like to find the right path moving forward.

I find this part controversial given how all other scripts work. It should work perfectly fine without the config file defined in the root of the project. This is how it is handled elsewhere. I understand that it makes it tricky to work with IDEs but it gives the possibility to run it from CLI on demand, so it still allows some automation.

I'd love to see some hybrid approach (probably applicable also for linters and test runners). When the script runs for the first time, it informs that the IDE integration is missing and asks whether to set up one. If the answer is yes then the config is created in the roof of the project. What do you think?

packages/scripts/scripts/format-js.js Show resolved Hide resolved
@@ -0,0 +1,3 @@
// Import the default config file and expose it in the project root.
// Useful for editor integrations.
module.exports = require( '@wordpress/scripts/config/.prettierrc.js' );

This comment has been minimized.

Copy link
@gziolo

gziolo Jan 15, 2020

Member

We should document it and ensure that this file is always usable outside. I guess it will become the blessed way to integrated with IDEs. Unless we expose a function that proxies the default configs provided.

const { getDefaultConfig } from '@wordpress/scripts';

module.exports = getDefaultConfig( 'prettier' );

It's probably unnecessary indirection though :)

@gziolo
gziolo approved these changes Jan 15, 2020
Copy link
Member

gziolo left a comment

I tested it locally again, it works like a charm. Gutenberg still works after all the refactorings applied :)

Let's get it in and work on the listed items in parallel to the PR which will format the whole codebase 💥

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 15, 2020

I almost forgot, which version of npm is required to use aliases? I think we should enforce it in Gutenberg at least.

Thinking about it a bit more, this is what WordPress core uses:
https://github.com/WordPress/wordpress-develop/blob/fc944f99279575ec9343c5ce3247b5b7f11ec78f/package.json#L10-L11

Is that enough? Otherwise, we won't be able to install @wordpress/scripts there ...

Edit:
I found this announcement: https://npm.community/t/release-npm-6-9-0/5911. We would need to enforce npm v6.9.0+

@jsnajdr

This comment has been minimized.

Copy link
Contributor Author

jsnajdr commented Jan 15, 2020

We would need to enforce npm v6.9.0+

Yes, it was also mentioned in this comment: #18048 (comment)

],
},
],
extends: [ require.resolve( './recommended-with-formatting.js' ), 'eslint-config-prettier' ],

This comment has been minimized.

Copy link
@ntwb

ntwb Jan 15, 2020

Member

Apologies for the lateness of this here, by removing the recommended config here and switching to recommended-with-formatting this may be troublesome for projects currently consuming @wordpress/scripts and using the ESLint config.

Will this affect those users in that the ESLint config is no longer available and they are not yet ready to switch to or know anything about Prettier?

p.s. I've not had the ability to study this use case in depth

This comment has been minimized.

Copy link
@jsnajdr

jsnajdr Jan 15, 2020

Author Contributor

@ntwb The recommended config is still available. This is how it works after this patch:

  • the recommended-with-formatting config includes all the rules that check code formatting, and that sometimes conflict with Prettier conventions. It's basically the former recommended config with a new name. It's intended for projects that want to opt-out from Prettier and continue to use ESLint for formatting.
  • the recommended config continues to exist, and it extends the recommended-with-formatting config, and disables all the code formatting rules by applying eslint-config-prettier. Therefore, the new default from now is to not check code formatting with ESLint, and use solely Prettier for code formatting.

Will this affect those users in that the ESLint config is no longer available and they are not yet ready to switch to or know anything about Prettier?

If you just upgrade to the new version of @wordpress/scripts and do nothing else, ESLint will stop reporting code formatting issues. Until you add Prettier to your project explicitly, not tool will be handing code formatting.

This comment has been minimized.

Copy link
@ntwb

ntwb Jan 15, 2020

Member

These changes should also be noted in the changelog

I'm still not sure on the idea of removing the ability for users to be able to format their code, need to think on this some more, I'm fully behind the switch to Prettier so by not doing this it may help in this regard and nudge users to add Prettier once they discover their code is no longer formatted 😏


Also, just looking at the code of the two files, instead of creating the new eslint-plugin/configs/recommended-with-formatting.js config file, why not leave the existing eslint-plugin/configs/recommended.js fille as is and add 'eslint-config-prettier' to extends in recommended.js?

As there are no other changes to the overall ESLint config besides this change and now that this is to be the default config anyways there isn't a requirement to add a new file for this is there? Thoughts?

e.g.:

	extends: [
		'eslint-config-prettier'
		require.resolve( './jsx-a11y.js' ),
		require.resolve( './custom.js' ),
		require.resolve( './react.js' ),
		require.resolve( './esnext.js' ),
	],

This comment has been minimized.

Copy link
@jsnajdr

jsnajdr Jan 17, 2020

Author Contributor

Also, just looking at the code of the two files, instead of creating the new eslint-plugin/configs/recommended-with-formatting.js config file, why not leave the existing eslint-plugin/configs/recommended.js fille as is and add 'eslint-config-prettier' to extends in recommended.js?

@ntwb I'm not sure I understand your comment -- I believe this is exactly what I'm doing in this PR. Rename recommended.js to recommended-with-formatting.js with the content not changed, and then create a new recommended.js that extends the -with-formatting one. The Git diff doesn't show the change as rename, but it's what happened.

This comment has been minimized.

Copy link
@jsnajdr

jsnajdr Jan 17, 2020

Author Contributor

These changes should also be noted in the changelog

Added an entry to packages/eslint-plugin/CHANGELOG.md

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 15, 2020

Related discussion on WordPress Slack in #core-js channel (link requires registration): https://wordpress.slack.com/archives/C5UNMSU4R/p1579085530105700

let’s make sure all WordPress build servers have npm v6.9 or later installed before we land Prettier changes so we accidentally don't break the workflow.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Jan 15, 2020

I added commit which enforces npm v6.9.0 or later: b6b5109. I also marked it as a breaking change for @wordpress/scripts.

jsnajdr and others added 8 commits Nov 14, 2019
Adds `eslint-config-prettier` to the recommended config and creates an alternative
`recommended-with-formatting` config for project that want to keep ESLint formatting
rules and opt-out of Prettier.
…op-level config

Prettier presence in a `wp-scripts`-powered project is optional, and the `format-js` script
checks if it's there and if it's indeed the fork (`wp-prettier`). Will report error otherwise.

Also, require a top-level Prettier config to be present. We can't default to the one inside
`wp-scripts`, because IDE and editor integrations won't find it there and will use the Prettier
defaults.
@jsnajdr jsnajdr force-pushed the add/prettier branch from b6b5109 to 0f2d90e Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.