Enforce Prettier (add CI and pre-commit hook)

[mchowning]

Fixes #816 by adding a prettier check to our CI builds and as a pre-commit hook.

This was tested by verifying that the "Check Correctness" check on CI failed before updating the code in this repo to comply with prettier, and the commit adding those changes caused the check to then succeed.

It may be easier to review this PR by looking at the commits so you can see the changes that are not solely prettier formatting updates separately. The vast majority of the changes in this PR are just formatting changes.

One mildly annoying aspect of this is the running prettier with the --list-different flag (which is needed to get a usable exit code) is that the console output seems rather unclear to me about whether it failed or succeeded ("success!" -> "error!"):

That is why I decided to also echo some error messages to the console and make it clear what was occurring. Certainly open to other thoughts though.

Update release notes

Not applicable because there are no user facing changes.

[jtreanor]

The CI changes look fine to me!

[JavonDavis]

@mchowning This looks good! just a quick question, should this pre-check auto format? The message says something like success formatting 1 file with prettier-eslint so it made me think the file the should have been updated but it wasn't

[mchowning]

Thanks @JavonDavis !

This looks good! just a quick question, should this pre-check auto format? The message says something like success formatting 1 file with prettier-eslint so it made me think the file the should have been updated but it wasn't

Yes, that is confusing, I completely agree 😀! This sounds similar to what I was talking about with respect to the screenshot in the issue description. It seems like this is the behavior when using the --list-different flag, which is needed to make the command return a non-zero exit code if there are any formatting changes. If I add the --write flag to also change the files, then the exit code becomes 0, so the build doesn't fail. 🤦‍♂️

Because of this issue, I did make the script on husky and circleci print an additional error message, so that it would be clear(er) that yarn prettier:check did not actually succeed:

"husky": {
      "hooks": {
          "pre-commit": "yarn prettier:check || { echo '\nERROR: `yarn prettier:check` found a problem\n'; exit 1; }"
      }
  }

I'm betting that you didn't see that because you ran yarn prettier:check directly (instead of having it run by husky or circle-ci). Since I only had the additional error messaging at the level of husky and circle-ci, you wouldn't have seen it running the command directly. I just pushed a commit (adcc5dd) fixing that so the additional error messaging is contained with the prettier:check script itself. Thanks for catching that!

FWIW, I also added a bit more detail to the error message to (hopefully) make it clearer. WDYT?

[JavonDavis]

@mchowning thanks for clearing that up for me, yeah I saw the error message and it was definitely clear but was just checking if I misunderstood what was entirely suppose to happen because of that first message I mentioned.

Now I'm wondering why we didn't go with the --write flag in the pre-commit hook since it would've done the auto-formatting and fix the errors, on CI we could leave as is to fail in case code code pushed without the pre commit hook being ran.

• Is it that we want to not change someone's code automatically? - They'd have to do it themselves anyway to pass the CI check so why not?
• Is it that the --write can't auto fix everything, in that case I'd still expect to fix what it can and return a non-zero exit code right?

I'm not sure but I think we could benefit more if we use the --write flag locally in the pre-commit hook, let me know if I'm making sense here or missing something, I haven't used prettier in a little while 😅

[mchowning]

I'm not sure but I think we could benefit more if we use the --write flag locally in the pre-commit hook

I chatted about this with some of the other gutenberg-mobile devs and I think the preference at this time is to not automatically make the changes. A couple of the reasons were (1) consistency with the gutenberg project currently just rejects the commit instead of making the changes automatically; and (2) @koke raised a good point about there possibly being unexpected behavior when not all changes are staged for commit. Personally, I'm not a fan of pre-commit hooks making changes to non-generated files, but that is probably more of just a personal preference.

I prefer to not automatically make any changes in the hook at this time. We can, of course, always update this if we decide we want that in the future. Let me know what you think @JavonDavis , and thanks for bringing this up!

[JavonDavis]

@mchowning Sounds good! I just caught up the conversation internally around it as well

Personally, I'm not a fan of pre-commit hooks making changes to non-generated files, but that is probably more of just a personal preference.

I kinda feel the same I mainly raised the question here since we'll be failing on CI if the check doesn't pass, so just thinking why not, in any case don't have strong feelings about this vs the way you currently have it so I think we're good to go for the merge IMO! 👍

[JavonDavis]

LGTM! I also completely agree with the approach to stay as close to how it's done in gutenberg as possible.

[maxme]

🎉