style(*): Automate Code Formatting #90

jshcrowthe · 2017-07-06T22:18:17Z

Adding in automation for prettier code formatting and running prettier on all the files.

NOTE: I have also removed the tslint.json file in this PR as I am going to add in some automated validation for code style after this commit. I will reintroduce the tslint.json and clean up the linter errors when I have an opportunity

mikelehen

This is probably fine, but what style is it using? It seems to be different than the internal google style for TypeScript (which e.g. prefers ' over "). It would be nice if we could be consistent so it's not too confusing for people that work in both worlds.

Also, why did you have to remove tslint.json? Is it conflicting with this style or something?

jshcrowthe · 2017-07-07T17:13:14Z

@mikelehen this is using the tool Prettier for formatting. Prettier is in the same realm of tools as clang-format only specifically tailored to TS/JS. It has some differences but is optimized for TS/JS (has supported for things like JSX/TSX which clang-format breaks, etc).

Considering this is automatically runs on commit, you can write your code however you want it all gets standardized for the review. If there is some specific things we don't like about the style we can address those for sure.

mikelehen · 2017-07-07T19:06:40Z

Fair point... I've been very impressed with clang-format so I'm biased towards it. I trust it to do the right thing in 99.8% of cases and I can live with the .2% where I'm annoyed with its formatting. I haven't used prettifier, so I'm nervous... and if we need to change settings or it changes behavior in the future, it'll create gratuitous diffs which make it harder to review code and pollutes the git history (so things like "git blame" are no longer useful).

But I'm not actually opposed. I spot checked a few files and don't see anything that really concerns me. So I can go ahead and approve....

mikelehen · 2017-07-07T19:07:32Z

Oh! One last comment. If folks have large outstanding PRs, this may create a merge conflict nightmare... something to keep in mind.

sphippen · 2017-07-07T20:32:45Z

The styling itself looks reasonable to me.

It's unclear to me why we shouldn't just leave the tslint.json file in though. Did the code pass all the lint checks before the reformat, but it doesn't afterward? Because if it didn't pass the checks before and it still doesn't afterward, I think we might as well just leave it there instead of having to add it back later when we address the lint errors.

jshcrowthe · 2017-07-07T20:39:56Z

The removal of the tslint.json is temporary as we weren't actually validating it and were failing all over the place when I actually checked it. I am going to do a linter fix PR in the coming weeks so that will reintroduce it. I'll automate validation of that as part of our CI (alongside prettier formatting).

tl;dr: It's only temporary till I can fix all the errors.

jshcrowthe · 2017-07-07T20:42:06Z

@mikelehen brings up a good point though. @jsayol seeing as you have the large open PR in question, would you like me to hold off till we can get your PR merged?

mikelehen · 2017-07-07T20:43:19Z

FWIW, fixing all the errors may be untenable. Internally we run tslint in such a way that it only complains about lines you actually changed. That way we don't regress, but it doesn't cause noise for all the existing violations (which grow over time as new rules are enabled, etc.)

jshcrowthe · 2017-07-07T20:45:23Z

Haha, fair enough. I will be trying to fix as many as possible and making it so the linter only runs on changed files is a great idea.

That said, I feel like, if we change tslint rules, that those should be accompanied by a fix for violating areas of the code.

mbleigh · 2017-07-07T20:47:11Z

I think we can configure prettier to prefer single quotes, no?

jshcrowthe · 2017-07-07T20:58:23Z

We can sure, if that's something we want it's super trivial to do.

mikelehen · 2017-07-07T21:08:00Z

It'd be interesting to run some code through prettifier and then clang-format (and vice-versa?) to see where they conflict and then see if we want to change any settings to make them more similar. If nothing else, I'm pretty curious to see if they are significantly different in any interesting ways. I'm okay if we don't want to invest the time for that though.

jshcrowthe · 2017-07-07T22:16:07Z

I could probably spin up a branch with clang-format, i'll get you a diff here in a bit

jsayol · 2017-07-07T22:38:43Z

The removal of the tslint.json is temporary as we weren't actually validating it and were failing all over the place when I actually checked it.

True that. I had to disable tslint in my local environment since my IDE was complaining about "errors" in almost every single line and it was unusable.

If folks have large outstanding PRs, this may create a merge conflict nightmare... something to keep in mind.
@jsayol seeing as you have the large open PR in question, would you like me to hold off till we can get your PR merged?

Yeah, that might actually be a good idea. It's already going to be quite a bit of work to rebase and solve conflicts as it stands right now (I'll get to it first thing tomorrow morning) but with all these other proposed changes it might turn into a bit of a nightmare. Thank you both for taking me into account, btw :)

Edit: I'm done rebasing PR #66.
There's still quite a bit of work left until I push the PR with persistence and I don't want to block this indefinitely, so go ahead with it. I'll merge and rebase that when the time comes.

jshcrowthe · 2017-07-08T21:14:03Z

@mikelehen I ran clang-format (default settings) on the codebase and pushed a branch, here is the diffs:

prettier: https://github.com/firebase/firebase-js-sdk/compare/prettier-styling
clang-format: https://github.com/firebase/firebase-js-sdk/compare/clang-format

I personally think prettier does a better job. But that's just my .02

jshcrowthe · 2017-07-08T21:25:45Z

We've got @jsayol's PR in and this branch has been re-pushed with his changes. Pending some stylistic adjustments (Full adjustment options are here: https://github.com/prettier/prettier#options) this should be G2G.

gauntface · 2017-07-10T11:10:10Z

Style seems generally fine to me, would keep the tslint in though.

mikelehen · 2017-07-10T17:08:23Z

I noticed a few things I'm not a big fan of:

Prettier seems to use 2 spaces for line continuation indents, while clang-format can use more (we use 4 internally, plus it'll sometimes use more to line up parameters, etc.).
Prettier has a heuristic where it will sometimes use ' and sometimes " for strings (to reduce escaping), even in a single line of code. clang-format opts for consistency instead.
Prettier seems to drop quotes on key strings in object literals, e.g. here: https://github.com/firebase/firebase-js-sdk/pull/90/files#diff-6f50da10b3fab2e68170ef0e0effffa9 ('TIMESTAMP': ... became just TIMESTAMP: ...) I'm not sure what we're using for minification these days, but with closure advanced compilation, this is likely to break our code since TIMESTAMP is now eligible for minification even though it's part of our public API and should not be minified.

Unfortunately none of these seem configurable. clang-format is much much more configurable it seems. I think item 3 may be a deal breaker (we need to figure out how to turn it off or verify that we're not minifying in such a way that it's impacted). But I don't feel strongly enough about the other 2 to push back too hard.

sphippen · 2017-07-10T17:24:06Z

I want to agree with @mikelehen on point 3 actually. When we stopped using Closure Compiler for storage the library went from ~30 kb to ~55 kb in uncompressed size, so I would like to introduce the Closure Compiler back into the storage build process at some point. Unless we have a way to keep keys in object literals as string literals, that doesn't seem possible.

jsayol · 2017-07-10T17:28:29Z

When we stopped using Closure Compiler for storage the library went from ~30 kb to ~55 kb in uncompressed size

@sphippen I'm actually working on a PR (#94) to reduce the size of the bundles. Preliminary results are good and I've made some more progress today. I may have something decent to merge either later today or tomorrow.

jshcrowthe · 2017-07-10T20:02:57Z

@mikelehen, I'll address point 3 because that seems to be the main blocker. We are currently using the package uglify-js to do minification. This internally does not do sub-property mangling by default. I have started looking into using Closure Compiler for minification but it seems like we have some things we'd need to fix first as the current version of closure compiler chokes on the webpack output.

Having said that, I've seen some stuff about using tsickle to help properly format the typescript output for closure so I'll look into something with that to see if we can get the built size down more. #94 gets us to a much better place and the beta v1 of the UglifyJS Webpack Plugin uses uglify-es which I've seen examples where it outperforms closure. So lots to investigate in this area.

jshcrowthe · 2017-07-24T22:35:42Z

@sphippen as of the release at 4.1.4, firebase-storage.js is back down to 33 kb uncompressed. In addition, I am going to try and standardize the underscore convention which would allow us to be much more deterministic with our mangling and prevent the overlap with the style tools.

jshcrowthe · 2017-07-24T22:37:46Z

All, if there are no more blockers on this I'm going to merge this and start working on the linter fixes and the underscore convention. Please let me know if you have any other concerns.

…in the build Something breaks in version 2.4.1 of typescript. We are pinning to 2.3.0 till those things can be triaged and fixed

Temporarily removing this file as we don't pass the checks

google-oss-bot added the needs-triage label Jul 6, 2017

jshcrowthe force-pushed the prettier-styling branch from 5b2f7e8 to 3fbdb87 Compare July 6, 2017 22:19

jshcrowthe changed the title ~~style(*): Leverage Prettier Code Formatting~~ style(*): Automate Code Formatting Jul 6, 2017

jshcrowthe requested a review from sphippen July 6, 2017 22:45

jshcrowthe requested review from gauntface, jdimond and mikelehen as code owners July 6, 2017 23:00

jshcrowthe requested a review from schmidt-sebastian as a code owner July 6, 2017 23:22

jshcrowthe added api: core api: database api: messaging api: storage and removed needs-triage labels Jul 6, 2017

mikelehen reviewed Jul 7, 2017

View reviewed changes

mikelehen previously approved these changes Jul 7, 2017

View reviewed changes

jshcrowthe dismissed mikelehen’s stale review via 3a3df33 July 8, 2017 21:03

jshcrowthe force-pushed the prettier-styling branch from cdce379 to 3a3df33 Compare July 8, 2017 21:03

jsayol mentioned this pull request Jul 8, 2017

refactor(database): Add types to several classes and utility methods #66

Merged

jshcrowthe force-pushed the prettier-styling branch from 3a3df33 to c361123 Compare July 24, 2017 20:26

jshcrowthe added 5 commits July 25, 2017 15:25

style(*): add prettier automation

9cb6392

fix(deps): fix issue where new typescript version was causing errors …

5a28b05

…in the build Something breaks in version 2.4.1 of typescript. We are pinning to 2.3.0 till those things can be triaged and fixed

WIP: remove tslint.json

511aeb5

Temporarily removing this file as we don't pass the checks

refactor(style): refaactor to prefer single quotes over double quotes

55cac08

style(*): run prettier on codebase

f95e1dc

jshcrowthe force-pushed the prettier-styling branch from c361123 to f95e1dc Compare July 25, 2017 22:28

jshcrowthe merged commit bc1f67c into master Jul 25, 2017

jshcrowthe deleted the prettier-styling branch July 25, 2017 22:53

firebase locked and limited conversation to collaborators Oct 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style(*): Automate Code Formatting #90

style(*): Automate Code Formatting #90

jshcrowthe commented Jul 6, 2017 •

edited

Loading

mikelehen left a comment

jshcrowthe commented Jul 7, 2017

mikelehen commented Jul 7, 2017

mikelehen commented Jul 7, 2017

sphippen commented Jul 7, 2017

jshcrowthe commented Jul 7, 2017 •

edited

Loading

jshcrowthe commented Jul 7, 2017

mikelehen commented Jul 7, 2017

jshcrowthe commented Jul 7, 2017

mbleigh commented Jul 7, 2017

jshcrowthe commented Jul 7, 2017

mikelehen commented Jul 7, 2017

jshcrowthe commented Jul 7, 2017

jsayol commented Jul 7, 2017 •

edited

Loading

jshcrowthe commented Jul 8, 2017 •

edited

Loading

jshcrowthe commented Jul 8, 2017

gauntface commented Jul 10, 2017

mikelehen commented Jul 10, 2017

sphippen commented Jul 10, 2017

jsayol commented Jul 10, 2017 •

edited

Loading

jshcrowthe commented Jul 10, 2017

jshcrowthe commented Jul 24, 2017

jshcrowthe commented Jul 24, 2017 •

edited

Loading

style(*): Automate Code Formatting #90

style(*): Automate Code Formatting #90

Conversation

jshcrowthe commented Jul 6, 2017 • edited Loading

mikelehen left a comment

Choose a reason for hiding this comment

jshcrowthe commented Jul 7, 2017

mikelehen commented Jul 7, 2017

mikelehen commented Jul 7, 2017

sphippen commented Jul 7, 2017

jshcrowthe commented Jul 7, 2017 • edited Loading

jshcrowthe commented Jul 7, 2017

mikelehen commented Jul 7, 2017

jshcrowthe commented Jul 7, 2017

mbleigh commented Jul 7, 2017

jshcrowthe commented Jul 7, 2017

mikelehen commented Jul 7, 2017

jshcrowthe commented Jul 7, 2017

jsayol commented Jul 7, 2017 • edited Loading

jshcrowthe commented Jul 8, 2017 • edited Loading

jshcrowthe commented Jul 8, 2017

gauntface commented Jul 10, 2017

mikelehen commented Jul 10, 2017

sphippen commented Jul 10, 2017

jsayol commented Jul 10, 2017 • edited Loading

jshcrowthe commented Jul 10, 2017

jshcrowthe commented Jul 24, 2017

jshcrowthe commented Jul 24, 2017 • edited Loading

jshcrowthe commented Jul 6, 2017 •

edited

Loading

jshcrowthe commented Jul 7, 2017 •

edited

Loading

jsayol commented Jul 7, 2017 •

edited

Loading

jshcrowthe commented Jul 8, 2017 •

edited

Loading

jsayol commented Jul 10, 2017 •

edited

Loading

jshcrowthe commented Jul 24, 2017 •

edited

Loading