Add Prettier - an opinionated code formatter

[simison]

• Adds a git pre-commit hook that automatically formats your code.
• Reformats the code in this same PR

Eslint rules still need work, since now "prettier" rules are applied everywhere.

https://prettier.io/docs/en/eslint.html

Resolves #858

Testing

• When you make changes to files and git commit, Prettier should process those files.

[simison]

Can be useful here: https://www.npmjs.com/package/pretty-quick

[ahstro]

What still needs to be done for this to be merged? Prettier is so convenient 😍

[simison]

What still needs to be done for this to be merged?

@ahstro I think the path for this starts to be clear; we've converted codebase to ES6, consolidated different Eslint configs and we're working on removing stuff from Gulp setup, too.

Wanna help out with this? :-)

[simison]

We have some choices of different strategies here — any preferences which way to go?

1. Aggressive reformatting on commit

• Each staging file (in git commit) would get automatically run by Prettier
• Those files would also get /* @format */ pragma added
• Your editor will run Prettier only on files marked with @format
• Everything just goes on automatically as we touch files (and someday we can reformat everything remaining)
• Causes more noise in PRs since lots of unrelated changes will be in each file

2. Manually reformat first

• Any file that we want in Prettier has to be formatted and committed to master first.
• We'll have /* @format */ pragma on those files
• Your editor will run Prettier only on files marked with @format
• Prettier will run for staged files on git commit only if a file has @format pragma, and otherwise keep them as-is
• More manual work up-front, less noise in PRs

3. Prettier everything

• Do a mega PR by Prettier-reformatting everything at once
• No @format pragma, Prettier always runs on all files (in your editor and on git commit)
• Just works — HUUUGE PR — Potential rebase nightmare for open PRs?

Thoughts?

[simison]

I tested with some files and unrelated Prettier changes are so much noise that I think we're better off with option 2. One big mega PR we can do once we're doing PRs mainly to Prettified files so that we don't end up in rebase nightmare. I'll changes settings in this PR to that effect. Lemme know if you have thoughts or prefer otherwise!

You can test the noise level by checking this branch, make some whitespace changes in any file and commit it and see the git log/diff.

[simison]

An alternative for lint-staged could be pretty-quick but its geared towards Prettier only. With lint-staged we can also run linter on files.

[nicksellen]

Yes, seems good to be able to have it doing general linting too. Although we also have linting in npm run lint:watch and in the webpack dev server... (for client files), linting everywhere... (not to mention my editor is doing it too).

Personally, I would not add the git hooks to my own local setup, I don't know why but they feel invasive, maybe I can learn to love them though. But not much unlinted makes it through the 3 lint stages above anyway.

[simison]

Hehe yeah. We should drop linting either here or in webpack dev-server+lint:watch. I like the simplicity of this setup and since I have continuous linting in editor, I don't really need the dev-server linting. I appreciate it might be easier to approach for new devs/volunteers, tho.

What do you think?

In future I'd like to add Stylelint to lint *.less files so it would get added here, too.

[simison]

Once everything is in Prettier, we might be able to remove these Eslint rules from our config, since Prettier-eslint config turns them off anyway:

• comma-dangle
• comma-spacing
• curly
• indent
• key-spacing
• keyword-spacing
• no-multi-spaces
• no-trailing-spaces
• object-curly-spacing
• one-var-declaration-per-line
• semi
• space-before-function-paren
• space-in-parens
• quote-props
• wrap-iife
• arrow-spacing

For now, we need to keep them since not every file is Prettified.

[nicksellen]

I actually don't mind mega PRs formatting change so much, I tend to find a way to run the prettier on my other PRs before trying to rebase/merge, so that it's more-or-less in the right shape anyway. Might be more fiddly getting the exact prettier config to use. But perhaps others are less happy doing PR fiddling, and so maybe your option 2 is best. I guess once we rebase our other PRs ontop, we could turn the @format thing on as we go...

[simison]

I'm happy to do mega PR if you all are fine with it. I don't have that much to rebase as you all have. ;-)

[mrkvon]

Rebased.

[mrkvon]

Interesting stuff! Never used this prettier before (if i don't count a random typescript tutorial 2 days ago).

How can we use this feature?

When i add the /** @format */ at the beginning of the ./modules/users/server/controllers/users.profile.server.controller.js and commit the change, i get a whole bunch of space-before-function-paren linter errors; as if the eslint and the prettier are fighting. I would expect it to do the formatting for me and finish gracefully.

And is it possible to format everything without adding the /** @format */ piece?

I don't have much idea about the 3 options of running prettier, but I like the option 3 for the reason that all the code is consistently formatted at all times. I haven't pondered it much though.

[simison]

And is it possible to format everything without adding the /** @Format */ piece?

Yes, I indeed changed it to that in the latest commit but didn't update the PR description yet. 🤦‍♂ I initially added format pragma just because I didn't want to do one mega-PR and choose portions of the code to convert in smaller chunks.

[mrkvon]

I squashed the commits until now, removed some conflicting eslint rules and performed the prettier reformat-files on md, json, html and js files.

A few notes:
The original branch (before force-update) is saved at branch prettier-backup.
I made Prettier <prettier@prettier.io> to be the author of those formatting commits. I don't want to have my name everywhere to git blame and i don't want to mess up my coding statistics either.

Are we ready for such a large change?

Please, review the additional removed eslint rules efb6b86, see that you're happy with the rest, too.

We'll still want to squash those additional eslint rules into the "configuration" commit and maybe squash together the reformatting commits as a single separate commit.

Please try to run the app and see that it still works as expected.

[mrkvon]

We may want to undo reformatting of md files in .github/ and html files in modules/core/server/views/email-templates-text/, and possibly more.

[simison]

Thanks, @mrkvon — on cursory looking Eslint config changes look good.

We may want to undo reformatting of md files in .github/ and html files in modules/core/server/views/email-templates-text/, and possibly more.

Should we just add those md & html files to eslintignore for now?

Alternatively, we could add <!-- prettier-ignore-start --> and <!-- prettier-ignore-end --> tags to those files.

What do you think?

[simison]

I can't press "approve" since I opened this PR but your changes look great!

Let's do this! 💪

I've spot-checked different files, tested linter locally with npm run lint and running this locally works.

Also IDE integration with both Eslint and Prettier seems to work:

When I modify the file, Prettier adds that comma back.

Also committing file makes Prettier add comma back.

When committing a js file and letting Prettier run on it, I see this:

⚠ Some of your tasks use `git add` command. Please remove it from the config since all modifications made by tasks will be automatically added to the git commit index.

...so suppose we can remove it. :-) OK to merge as is and remove later, or if you remove please test that committing+Prettier still works.

[mrkvon]

I suppose the git add is necessary if we want the Prettier+commit work. (Haven't tested.)

We get warned rightfully. I find it scary indeed that there are changes which get committed and which a developer has no control over. It's just something I'll need to get used to. 🙂

I wonder whether to squash the four formatting commits into one (md, html, js, json). I'm inclined to do it, but I'd like a second opinion. Probably doesn't matter anyways. 🙂

[mrkvon]

I've squashed the commits a bit, so there are 2 commits left: config and formatting. I think it would make sense to keep it that way and use the Rebase and merge button.
If everything seems ok, @simison feel free to merge. 🙂
Otherwise I'll merge in 12 hours or so. gn.

[simison]

One huge reformat commit seems fine instead of splitting by format.. Might make rebasing easier?

I also thought precommit hook needs the git add but the warning made me think it might make it on our behalf?

[simison]

The idea that commits Prettified files on your behalf is to provide convenience. :-) I have it set in editor myself, so it reformats as I type or safe files.

You'll still get chance to see the edits before pushing and then finally before merging.

[mrkvon]

@simison Indeed, your understanding is correct. I've just tested that with the git add removed from lint-staged config, the prettier changes are still committed.

The change was included with the latest push --force

[mrkvon]

With that done, let's merge.

Kudos @simison 🎉

And let's see how rebasing after this will go...

[simison]

Perfect! 👍 Thanks for testing the git-thing.

Feel free to merge.