Adding Prettier on Commit #580
Conversation
Updating yarn lock
schmidt-sebastian
force-pushed
the
mrschmidt-prettier
branch
from
8a2b9a7 to
8cf8cec
Compare
|
BTW, this adds 86 (!) new indirect dependencies. The size of node_modules increases from 615 MB to 626 MB (we still fit on a CD-R...). |
There was a problem hiding this comment.
While I'm very much in favor of improving the prettier workflow (performance especially), I'm not sure I'm wild about this change... I still want to be able to see what prettier does to my code (either by putting it in a separate commit or just making it a manual step that I do on my own).
FWIW- I opened an issue about prettier improvements a while ago here: #531 (the other big thing I think we need to tackle is the versioning mismatches we keep hitting... though we can deal with that separately.)
| @@ -17,6 +17,7 @@ | |||
| ], | |||
| "scripts": { | |||
| "dev": "lerna run --parallel --scope @firebase/* --scope firebase dev", | |||
| "precommit": "lint-staged", | |||
There was a problem hiding this comment.
Oh boy... I don't know if I trust prettier enough to let it silently modify my commits. Then I can't tell what it did and I might not notice problems. Today I always run "yarn prepush" manually, check what it did, and then if I'm satisfied, I'll clean up the git history via "git rebase -i master" or whatever.
There was a problem hiding this comment.
Using the staged files makes it really easy to figure out what files to re-format. A while ago, I tried something similar to what you suggested in #531 (diff against origin & upstream) but wasn't able to get it to work properly in between my other work.
I personally don't like the Prettier commits but I also don't want to take them away from you (since I can just rebase if need be). Maybe there is some way we can still stage them in a separate commit (by not passing --write, or not doing git add?). Do you have suggestions for this, @jshcrowthe?
I do want to ask though if it is even possible to disagree with prettier. If you manually re-format prettier's changes, doesn't it just revert these changes on the next run?
There was a problem hiding this comment.
I don't know what this would entail, but we could do a post-commit hook and just try and format the last diff? I dunno what it would require/look like, but it seems doable.
There was a problem hiding this comment.
I sometimes choose to wrap lines differently (or restructure code to avoid wrapping, etc.). And I just like to know what prettier is doing. We've run into cases where it just does wonky reformatting (either due to bugs or poor design) and I like to notice those when they happen.
| }, | ||
| "lint-staged": { | ||
| "*.ts": [ | ||
| "prettier --write", |
There was a problem hiding this comment.
prettier.js references repo-root/.prettierrc. Are we losing that?
There was a problem hiding this comment.
Just verified that this still works.
| const fs = require('mz/fs'); | ||
| const glob = require('glob'); | ||
| const simpleGit = require('simple-git/promise'); | ||
| const ora = require('ora'); |
There was a problem hiding this comment.
Do we still want / need this script to fix up the whole repo after upgrading prettier?
There was a problem hiding this comment.
We won't need it necessarily no. It can always be run manually by just calling ./node_modules/.bin/prettier and I'd imagine we do so fairly rarely.
There was a problem hiding this comment.
It should also be an explicit step for these kinds of upgrades.
There was a problem hiding this comment.
Yeah... I more meant "Do we still need a script that runs prettier on the whole repo?" since the commit hooks aren't going to catch everything (rebases, amend commits, post-prettier-upgrade reformats, etc.)?
Maybe it's simple enough to just run it from the command-line manually though. I'm not sure what all this script was doing...
There was a problem hiding this comment.
This script just runs prettier on the whole SDK and gives you progress updates. So we could probably just make an NPM script in the repo root to take care of the global style hammer.
|
So it's interesting that this was proposed as it was actually the original way this was done. We had originally changed to the
Now, with those things on the table, I'm largely in favor of this change (like I said it was how it was done originally). It is fairly common amongst folks using prettier and I'd love to see us follow that convention. There are workarounds for the issues above (e.g. commiting with the |
|
Closing this in favor of #585. Thanks Josh. |
I should have discussed this per email first, but writing this in form a PR was a lot quicker.
This PR addresses two of my issues that I see with the JS SDK development:
It is hard to run formatting on the code on the fly. I have to commit my code, then run the githooks manually from the repo root. I now have a commit in my commit history that I didn't necessarily want.
Formatting all source files on
git pushtakes too long. We should speed this up by only formatting the files that changed.The common way to do this seems to be via
lint-staged, which is actually so fast that we can run in on commit. Commit times for me with this change are < 1.5 seconds for small sets of changes.