Proposal to replace Clang Format with ESLint and Prettier in phosphor-webui

Proposal to replace Clang Format with ESLint and Prettier in phosphor-webui

[Derick]

Hello,

I propose we replace Clang Format in the phosphor-webui with an ESLint
and Prettier configuration. Clang format handles formatting reasonably
well, but it doesn't offer the ability to help identify potential
errors.

We can run ESLint and Prettier from the command-line and integrate
them into our editors. I have included links to the documentation for
the resources and would like to continue the conversation in the
Gerrit (https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-webui/+/22366).

My initial goal was to use the Angular 1 Style Guide's shared eslint
and the ESLint recommended configurations. However, those options
require a considerable amount of effort to update and regression test
to be compliant. For now,  I have added a few Angular and ESLint rules
to the configuration that are categorized either as possible errors or
best practices. If we decide the AngularJS community's suggested best
practices will improve our code base, we can develop a plan to
implement those changes in a phased approach.

- ESLint: https://eslint.org/
- Prettier: https://prettier.io/
- ESLlint Angular Plugin: https://www.npmjs.com/package/eslint-plugin-angular
- Angular 1 Style Guide:
https://github.com/johnpapa/angular-styleguide/tree/master/a1
- Gerrit Review:
https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-webui/+/22366

Re: Proposal to replace Clang Format with ESLint and Prettier in phosphor-webui

[Ed Tanous]

On 6/6/19 7:37 PM, Derick wrote:
> Hello,
> 
> I propose we replace Clang Format in the phosphor-webui with an ESLint
> and Prettier configuration. Clang format handles formatting reasonably
> well, but it doesn't offer the ability to help identify potential
> errors.
> 
> We can run ESLint and Prettier from the command-line and integrate
> them into our editors. I have included links to the documentation for
> the resources and would like to continue the conversation in the
> Gerrit (https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-webui/+/22366).


In reading the last couple paragraphs, I think we really need a good
statement of "why" we want this change.  I think we really need to
separate out ESlint, and Prettier as separate tools, with specific
reasons for using them.

For what it's worth, my "why" for ESlint was that it would make webui
code easier and faster to code review for correctness.  I'm not sure
ESLint really accomplished that goal.

ESLint:

My 2 cents:  As I'm sure you're aware, I started an attempt to get
ESlint going on the webui.  It was a good experiment, but overall, I'm
not convinced of its value in the value/time equation.
https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-webui/+/15968

If I look through the changes that got made to make Eslint pass, I'm was
not seeing a lot of value for the time.  Most of it seems like brace
styling, semicolon policies, and debates about var vs let vs const
variable declarations.

Don't get me wrong, I'm all for consistent braces, but I don't think
it's the biggest issue we face, or really worth the time it would take
to get the builds working and passing.  Very rarely do we need to rework
the webui, tackle bugs, or revert patches because of those issues.

If ESlint is really something you want to tackle, by all means, but in
terms of value to the end user, I suspect it's a wash.

Prettier:
I struggle with this one, because as a whole, this project seems to use
clang-format for most everything else.  In the research I've done, I
wasn't able to find something that Prettier does significantly better
than clang-format, with the exception of maybe being more "standard" in
the javascript realm.  I'd much rather OpenBMC sticks with something
that's consistent than say every language gets its own formatting library.

> 
> My initial goal was to use the Angular 1 Style Guide's shared eslint
> and the ESLint recommended configurations.

If you start with eslint-config-google, the changes are a lot less
invasive, as we're pretty close to being compliant.

> However, those options
> require a considerable amount of effort to update and regression test
> to be compliant. For now,  I have added a few Angular and ESLint rules
> to the configuration that are categorized either as possible errors or
> best practices. If we decide the AngularJS community's suggested best
> practices will improve our code base, we can develop a plan to
> implement those changes in a phased approach.

This is some of what stopped me.  If we're going to go to this level of
effort, my gut said we're a lot better off going to typescript and
Angular 2.X+, rather than continuing to rework our angular 1.X stuff to
use more "best practices" on the less-than-up-to-date stack we're using.
 At one point I had looked at moving forward with Angular 2 and
typescript, and tried to sketch out the path to get there, but it
quickly got out of hand.


> 
> - ESLint: https://eslint.org/
> - Prettier: https://prettier.io/
> - ESLlint Angular Plugin: https://www.npmjs.com/package/eslint-plugin-angular
> - Angular 1 Style Guide:
> https://github.com/johnpapa/angular-styleguide/tree/master/a1
> - Gerrit Review:
> https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-webui/+/22366
> 

Re: Proposal to replace Clang Format with ESLint and Prettier in phosphor-webui

[Derick]

Thanks for the follow up Ed!

> Prettier:
> I struggle with this one, because as a whole, this project seems to use
> clang-format for most everything else.  In the research I've done, I
> wasn't able to find something that Prettier does significantly better
> than clang-format, with the exception of maybe being more "standard" in
> the javascript realm.  I'd much rather OpenBMC sticks with something
> that's consistent than say every language gets its own formatting library.

I am ok if we don't want to use Prettier for JavaScript, but
clang-format does not
support SCSS and we would like to have consistent formatting in SCSS as well.
We wouldn't expect it to stop the builds, just to ensure code quality. I want
to use the .prettierrc file to keep it consistent for anyone that uses
Prettier and
we can easily ignore .js files in the config. We can also make this a
work in progress
if we don't want to try and resolve all the files up front.

> If ESlint is really something you want to tackle, by all means, but in
> terms of value to the end user, I suspect it's a wash.

I feel the benefit to the user is code quality and to the developer it
is efficiency.
You're correct, the formatting is not something that results in bugs that
require rework. However, clang-format does not surface any potential errors.
It is simply a formatting tool and not one that is widely used in the JavaScript
community (https://www.npmtrends.com/clang-format-vs-eslint-vs-prettier).
ESLint is extremely helpful to people using IDE/Editors like Visual Studio
Code due to it's integration and showing the developer potential problems
in their code as they are writing it. Personally, I have found the
formatting of
ESLint with the Google shared config preferable than clang-format and more
consistent with the formatting most JavaScript projects use.

> If you start with eslint-config-google, the changes are a lot less
> invasive, as we're pretty close to being compliant.

We want to use ESLint based on its ability to catch errors. I did
start with Google's
shared eslint configuration file and it is less invasive. There are
still a few issues to
resolve with that configuration, but if that is the tradeoff for using
ESLint, I'm good with that.

> At one point I had looked at moving forward with Angular 2 and
> typescript, and tried to sketch out the path to get there, but it
> quickly got out of hand.

Agree, that will require effort and time and our team doesn't have the
bandwidth for that.

Re: Proposal to replace Clang Format with ESLint and Prettier in phosphor-webui

[Ed Tanous]

On 6/14/19 10:12 AM, Derick wrote:
> 
> I am ok if we don't want to use Prettier for JavaScript, but
> clang-format does not

Never really thought of that, and that's a fair criticism of using
clang-format for the web stuff.  I do have prettier hooked into my
editor for the scss files, so I guess I never really thought about it.

> support SCSS and we would like to have consistent formatting in SCSS as well.
> We wouldn't expect it to stop the builds, just to ensure code quality. I want
> to use the .prettierrc file to keep it consistent for anyone that uses
> Prettier and
> we can easily ignore .js files in the config. We can also make this a
> work in progress
> if we don't want to try and resolve all the files up front.
> 
>> If ESlint is really something you want to tackle, by all means, but in
>> terms of value to the end user, I suspect it's a wash.
> 
> I feel the benefit to the user is code quality and to the developer it
> is efficiency.
> You're correct, the formatting is not something that results in bugs that
> require rework. However, clang-format does not surface any potential errors.
> It is simply a formatting tool and not one that is widely used in the JavaScript
> community (https://www.npmtrends.com/clang-format-vs-eslint-vs-prettier).
> ESLint is extremely helpful to people using IDE/Editors like Visual Studio
> Code due to it's integration and showing the developer potential problems
> in their code as they are writing it. Personally, I have found the
> formatting of
> ESLint with the Google shared config preferable than clang-format and more
> consistent with the formatting most JavaScript projects use.

Maybe this is some of the disconnect.  When I went through this
exercise, the only thing it was doing was bumping some braces around to
different locations, which is already consistent across the project.  I
didn't really see a significant impact.  If you think the google config
makes you more efficient, by all means, I can get behind it, that just
wasn't my experience.

> 
>> If you start with eslint-config-google, the changes are a lot less
>> invasive, as we're pretty close to being compliant.
> 
> We want to use ESLint based on its ability to catch errors. I did
> start with Google's
> shared eslint configuration file and it is less invasive. There are
> still a few issues to
> resolve with that configuration, but if that is the tradeoff for using
> ESLint, I'm good with that.

This is kind of what I'm talking about.  I had the same hope, but I
didn't really see a lot of errors being caught that would've been a real
production issue.  With that said, it looks like you turned on a heck of
a lot more checks than I did.  Did you find any patterns that the UI
consistently got wrong?  Were there checks that identified real
functional issues in the webui (memory leaks, bad pages, race
conditions, ect)

I'm scrolling through your changeset, and most of it seems like
whitespace.  The brace rules are different.  Object keys seem like
they're always quoted.  CSS pixel alignments take the form of "0.6em"
versus ".6em".  Not a lot of these improve the readability a lot IMO.

> 
>> At one point I had looked at moving forward with Angular 2 and
>> typescript, and tried to sketch out the path to get there, but it
>> quickly got out of hand.
> 
> Agree, that will require effort and time and our team doesn't have the
> bandwidth for that.
> 
Yep.  I wasn't asking your team to do it.  I was more pointing out that
a lot of the stuff that I was really wanting out of this exercise:
(static analysis, transpiration correctness guarantees, type safety,
increased code review efficiency with automation), were only available
or useful when used from a typescript application.


All in all, if this is something you want, I can get behind it.

Re: Proposal to replace Clang Format with ESLint and Prettier in phosphor-webui

[Derick]

>  If you think the google config
> makes you more efficient, by all means, I can get behind it, that just
> wasn't my experience.

Where I find it valuable is the visual indicators that happen when
integrated into the editor. It helps hunt down small problems that
often cause big issues, but are hard to spot.

> Did you find any patterns that the UI
> consistently got wrong?

Mostly  just inconsistencies in formatting, but there were some
syntax errors that prettier just fixed for us, unnecessary semi-colons
is one example.

> Were there checks that identified real
> functional issues in the webui (memory leaks, bad pages, race
> conditions, ect)

No

> I'm scrolling through your changeset, and most of it seems like
> whitespace.

It was mostly with exception to some syntax errors. In the update
to the Gerrit review, I removed all the files I formatted, so running
eslint on the app folder will reveal them. I will go in and resolve
those issues in a different patch.

> All in all, if this is something you want, I can get behind it.

Thank you! I don't want to stretch this conversation out. I have found
a win-win.
We can keep the formatting in place using clang-format. ESLint can provide
basic fixes, like removing trailing commas and flagging syntax and potential
issues without impacting the build. We can use prettier only for SCSS.

Re: Proposal to replace Clang Format with ESLint and Prettier in phosphor-webui

[Derick]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

Thanks Ed for the reviews and support on the GUI tools work! I ran into a
roadblock with the format-code.sh call. I am not finding Prettier as an
Ubuntu
package and won't be able to make the call during the build process.
However,
I am able to use a Git hook to test the code prior to commit and/or push.

I added the npm scripts that can be called for testing and fixing (if
auto-fixable)
so that they can be run from the command line for anyone that doesn't
Prettier
integrated with their code editor (it even has VIM support).

 I have these changes in the Gerrit review
https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-webui/+/22366
and am hoping that this is an acceptable solution.

[-- Attachment #2: Type: text/html, Size: 952 bytes --]

Re: Proposal to replace Clang Format with ESLint and Prettier in phosphor-webui

[Ed Tanous]

On 7/10/19 10:17 AM, Derick wrote:
> Thanks Ed for the reviews and support on the GUI tools work! I ran into a
> roadblock with the format-code.sh call. I am not finding Prettier as an
> Ubuntu
> package and won't be able to make the call during the build process.

Not surprising;  Prettier is an npm package, not specific to Ubuntu.

sudo npm install --global prettier

Should give you the result you want.

> However,
> I am able to use a Git hook to test the code prior to commit and/or push.

That's great that this is available, but I'm not really comfortable if
it can't be enforced in CI.  That would mean someone needs to pull down
each commit and verify it manually.  That doesn't really scale IMO, and
considering that the incumbent (clang-format) can already integrate
properly, it's hard to justify going backwards.

> 
> I added the npm scripts that can be called for testing and fixing (if
> auto-fixable)
> so that they can be run from the command line for anyone that doesn't
> Prettier
> integrated with their code editor (it even has VIM support). 

They'll still need to have it installed globally, which could be added
as part of the main webui install, but I'm not sure that would be worth it.

> 
>  I have these changes in the Gerrit
> review https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-webui/+/22366
> and am hoping that this is an acceptable solution. 

My main sticking point is the significant manual testing that's going to
have to take place, for a relatively modest increase in productivity.
If that can be resolved, I would support you moving this change forward.