Go: x/tools: agree to use a specific JS formatter

[carlmjohnson]

I recently worked on #21643, which involved changing the JavaScript in the x/tools repo. This JavaScript has no clear guidelines for style and is written in an inconsistent way from file to file. Some of the semi-colons, e.g., in slides.js are redundant, and semi-colons are missing in other places.

I propose that we add a JavaScripter formatter and linter to the repo to remove these inconsistencies and prevent their recurrence. Prettier could be used as a JavaScript equivalent of gofmt, and ESLint could be used to prevent issues like unused variables, unintended global variables, etc.

[carlmjohnson]

@andybons expressed interest in this while reviewing a CL for #21643.

[OneOfOne]

Please use tabs instead of spaces to match gofmt, please, please, please.

[andybons]

Oh no what have we done

[rsc]

Has the JavaScript community agreed on a standard formatter?

I don't see a reason for us to check one into our repo. It would be fine - if everyone agrees - to have a soft standard that maybe the JS should be formatted by , best effort.

There are only 21 JS files in golang.org/x.

Leaving for @andybons to approve a specific formatter (or lead a discussion toward approving one).

If the proposal is just 'agree that we should format JS using tool X' and not 'force this in some kind of presubmit or automated tooling', then proposal accepted, pending @andybons and others picking a formatter.

[agnivade]

Coming here from #24933. The 2 most popular tools in js linting and formatting space is standard js and eslint:recommended. We can decide on any one of them and just go with it.

But my biggest complaint is that using these tools will involve the entire npm ecosystem. You will need a nodejs installation, package.json, node_modules need to be added to .gitignore and all the baggage that comes with it. With just a handful no. of js files in our repos, I don't see a need to check these in the repo.

Maybe we can come to a middle ground and document what formatter we use. The developer just runs the tool manually before pushing a CL. And once before every release, somebody can run a manual check to confirm if everything is according to standard, and if not, push a separate check-in. Or something like that.

[carlmjohnson]

Or someone could write a JS code formatter in Go. :-)

[opennota]

standard.js removes semicolons by default (which is probably not what we want); and eslint:recommended wouldn't complain about lack of spaces around operators: var a =1+ 2;

And the up-and-coming prettier formatter is more or less like gofmt.

[carlmjohnson]

prettier is to gofmt as eslint-recommended is to go vet.

[agnivade]

Looks like I have fallen behind on the latest JS tool-of-the-week. Took prettier out for a spin. I am good with it. Only minor change that is needed is that we need to pass a flag to use tabs, because it uses spaces as default. A combination of prettier and eslint:recommended for formatting and vetting sounds good to me.

[OneOfOne]

Prettier's extremely opinionated defaults that you can't change are annoying, I ended up just using eslint + airbnb's config + sane custom settings that resemble gofmt.

My config is more es6/ts though, but I'd gladly help and work with anyone if they wanna adapt it / change it.

https://gist.github.com/OneOfOne/c62e964cc159c22f36ce5dd535de2270#file-eslintrc-js

@carlmjohnson while that's kinda true, you can 100% just use eslint for formatting without prettier.

[agnivade]

I ran it on a couple of js and css files. Nothing felt too annoying to me. Everybody has their own opinions on what is annoying and not. Even I recommended initially to go with eslint / standard.js.

Whatever we choose, we wouldn't want to have configuration to customize the rules. I think the point is to just choose one and go with the defaults. Maybe one or two customizations at most (like tabs instead of spaces); not more than that.

[agnivade]

Ok, spent some time and evaluated these tools in some more detail.

I have kept the following rules in mind during the evaluation process:

• Should need bare minimal config knobs.
• Attempt to conform to Go code style.
• Overall, it should follow the Go philosophy of simplicity and minimalism.

Tools evaluated:

• Standard.js - Formats only js files. Uses a fixed set of eslint rules under the hood.
• Eslint:recommended - Main job is to function as a linter. The "recommended" rule set contains a default set of rules to be checked against.
• Prettier - Used purely as a formatter. Operates on a lot of languages including js/css. Doesn't do linting.

Points to be noted:

• Both standard.js and prettier use spaces by default. Standard.js doesn't expose a way to customize that (you can customize the eslint rule underneath, but that doesn't change the formatter; at least from what I have seen).

• Standard.js removes semicolons, prettier doesn't by default (can be customized).

Given all the above, prettier is the closest alternative to gofmt. And to remain as close as possible to the Go-style of code, it just needs 2 knobs - --use-tabs and --no-semi.

I propose we go ahead with prettier as the formatter.

Regarding linting, we just need something to print out the failures against a fixed rule set. eslint:recommended fits that role nicely. But the primary focus in these repos is to write Go code. There are just a handful of js files, and proposing guidelines which cannot be automatically fixed by developers is frankly a waste of time for so little js code. I have actually run eslint:recommended on the codebase, and it mostly gives errors for undefined global variables. A minor thing to consider given the fact that it's majorly a Go codebase.

I would recommend not to use any linting at all, or at most mention a one-liner that it is an added bonus if your js code passes eslint:recommended.

PS: Although you can use prettier along with eslint to do both formatting and linting, you need to take care of conflicting rule sets. And then further packages need to be installed to take care of that (https://prettier.io/docs/en/eslint.html). I feel this only complicates things and prevents a clean separation of concerns between code formatting and vetting.

I have already run prettier on the godoc repo and the resulting output looks pretty clean and neat. The resultant code size increases slightly because of the added newlines and tabs. But once we start using a minifier (which I have plans for !), this should be taken care of.

Over to @andybons for a final decision.

[jimmyfrasche]

Is removing semicolons just to make the js more like Go code? I don't see the point and semicolon insertion in js is fraught.

I can see sticking to tabs consistently to avoid having to toggle soft tabs on and off in editors when moving between languages in the same codebase and it being hard to notice if you did not, though.

[agnivade]

Is removing semicolons just to make the js more like Go code?

Yes. IMO, the code looks much cleaner. Although this is pure bikeshedding territory. We should just choose one and go with it.

[jimmyfrasche]

If the goal is to avoid bikeshedding, then we should just accept all of the tool's default settings unless there's a pragmatic argument for requiring a specific setting.

[andybons]

Sorry, but we’re not removing semi-colons from JS.

JS is not Go. We shouldn’t make decisions in the interest of making the former look like the latter.

Rather than spending time debating the merits of specific knobs to turn, I’d rather have solutions for when a dev doesn’t want to install npm and a gillion dependencies on their machine in order to format their code.

Let’s please focus on that particular problem.

[agnivade]

JS is not Go. We shouldn’t make decisions in the interest of making the former look like the latter.

Cool, no worries.

I’d rather have solutions for when a dev doesn’t want to install npm and a gillion dependencies on their machine in order to format their code.

I would love that too. Unfortunately, I don't see any js/css formatter written in Go.

[jimmyfrasche]

There's also version skew in the formatter to consider. If I'm using version X and you're using version Y, they may format the same code differently.

A complete solution would be to have a bot that formats CLs which would always be a single fixed version and configuration (and set to ignore vendored resources like jquery).

It would probably suffice—at least for now—for someone to run the formatter on all relevant sources once and then afterwards we just endeavor to make new code look like old code and call out dissimilar formatting in reviews.

[agnivade]

There's also version skew in the formatter to consider. If I'm using version X and you're using version Y, they may format the same code differently.

Yep, right. It would be mentioned in the README what version to install. Since we wouldn't be checking in package.json.

It would probably suffice—at least for now—for someone to run the formatter on all relevant sources once and then afterwards we just endeavor to make new code look like old code

I was suggesting to go one step further and just run the formatter from time to time to keep the codebase clean. Code reviews are difficult as it is, we wouldn't want to add additional load of reviewing style too.

Regarding the issue of installing the node dependencies, my thinking was that since this is not a hard requirement in the first place, nobody is _required_ to install node/npm at all. Just the install steps will be mentioned in the README (Assuming we will be using the node ecosystem in the first place). If someone is willing to do it, great, otherwise no hard feelings.

I am willing to take point in running the formatter probably once a month to check and see if anything has fallen out of line.

[andybons]

Adding an additional step that must be performed by a single person every month seems like a step backwards from adding a formatter in the first place.

It also is just churn and muddies up git blame.

Many of the devs that do development on these products may have docker installed. Could we dockerize the command so that the user only needs to download an image and run that? See https://spin.atomicobject.com/2015/11/30/command-line-tools-docker/

[agnivade]

It also is just churn and muddies up git blame.

Hmm .. how does actually installing node/npm, vs running it through docker make a difference to the git blame ? The code change will still be the same right ?

Personally, I feel installing docker to be an extra mental overhead. It reduces my ability to debug things. But I can see how it can be easy for some users. If you agree that we should use prettier, I can probably investigate and see if it works out.

[andybons]

Dockerizing the command and the git blame issue are separate concerns. Having someone go through the codebase every month reformatting things doesn't scale, has a bus factor of 1, and will muddy up git blame. The docker issue is separate from this.

I should have said "_provide an option_ to download a dockerized version of the command so that those who don't want to download npm, node, and the world will have an easy way of avoiding those steps."

[kelwang]

also need to bypass minified js

[agnivade]

Docker image agniva/prettify at your service.

docker pull agniva/prettify

13:21:41-agniva-~/play/go/src/golang.org/x/tools$docker run --rm -it --volume "$PWD":/wd  agniva/prettify "**/*.{js,css}"
cmd/present/static/article.css 155ms
cmd/present/static/dir.css 35ms
cmd/present/static/dir.js 35ms
cmd/present/static/notes.css 9ms
cmd/present/static/notes.js 40ms
cmd/present/static/slides.js 137ms
cmd/present/static/styles.css 104ms
godoc/static/godocs.js 119ms
godoc/static/play.js 28ms
godoc/static/playground.js 88ms
godoc/static/style.css 106ms

Once you alias it to the prettier command - alias prettier='docker run --rm -it --volume "$PWD":/wd agniva/prettify'. Then it becomes -

$prettier "**/*.{js,css}"

Source - https://gist.github.com/agnivade/5bc1987c4ef954eb63627679849c9da5

[agnivade]

@andybons - Now that work is going on with the website, any thoughts on this ? I guess you are already manually using prettier for the website css ? Maybe we can formalize a process around it while you are at it.

[andybons]

If I were starting a new project, the following would be in the .prettierrc file:

{
  "singleQuote": true,
  "trailingComma": "es5"
}

[agnivade]

Sure, that can be done. I was asking more on how can we actually move ahead and formalize this. Should I push the Dockerfile to a repo ? And I guess the instructions to run the formatter should just be a note in the README ? Any places other than x/tools and x/website that needs this ?

[andybons]

Let’s start by documenting it and if someone wants to do a pass on all the JS that’s fine. I’m less worried about git blame now as long as it’s not part of an unrelated change.

[gopherbot]

Change https://golang.org/cl/183877 mentions this issue: all: add a section on JS/CSS formatting to README

[gopherbot]

Change https://golang.org/cl/183878 mentions this issue: all: run prettier on js and css

[gopherbot]

Change https://golang.org/cl/183879 mentions this issue: all: add a section on JS/CSS formatting to README

[gopherbot]

Change https://golang.org/cl/183880 mentions this issue: all: run prettier on js and css

[andybons]

I apologize for the delay on reviewing the CLs here. I’ve been thinking about this a lot.

In practice, people simply don’t read (despite our best efforts). We should keep that in mind when we introduce this requirement.

My first inclination is to check in the prettier config and encourage the small group of people making changes to the JS/CSS to configure their editor accordingly.

Is there a way to specify that we only want to format .js and .css files for now? Go templates get messed up when prettier thinks they’re html (and I’d rather not change our suffixes in some cases)

[carlmjohnson]

prettier --ignore-path .gitignore --check **/**/*.{css,js,yml}?

[agnivade]

Is there a way to specify that we only want to format .js and .css files for now?

Yep. See my CLs which change the READMEs.

Just prettier "**/*.{js,css}" works. We can specify more knobs in the .prettierrc. The CL has more details.

[agnivade]

The READMEs of both the website and tools repos are updated now, along with a checked-in .prettierrc file for everyone to use. It is encouraged that developers use it to format their code, but it's not enforced in any way.

With that, I am calling it a close. Thanks everyone for the discussion.