[RFC] Lint codebase using standard

[tvararu]

Depends on:

• Remove / refactor some variable declarations #223
• Remove selection-buttons.js #233

This PR is an open discussion on the topic of using a JavaScript linting tool across our codebases.

I throw my hat in the ring by suggesting standard and demonstrating with this PR how an implementation would work for a node project.

Code was automatically converted using https://github.com/maxogden/standard-format.

We already have govuk-lint whose responsibility it is to handle linting concerns, and I'd love to hear what our thoughts are on relying on that instead.

[NickColley]

Shouldn't need to do this. twbs/bootstrap#3057

[paulmsmith]

I saw this tweet recently: https://twitter.com/brendaneich/status/741277499376603138 - Tim Berners-lee & Brendan Eich use standard.

[joelanman]

thanks for this, sorry I dont have much experience on this - can you explain a bit more about how it works?

[tvararu]

@joelanman certainly!

1edbe61 is where I've added standard as a project dependency. standard is an opinionated JavaScript linter that uses eslint underneath and acts as a set of sensible defaults for it; here are the most important rules.

Since standard is based on eslint, overriding rules inside a file / ignoring files works through the same mechanisms, and if we want to move away or build on top of its opinionated ruleset, we can easily do so by swapping it with eslint and dropping an .eslintrc file.

Also worth mentioning some other "batteries-included" linters that fork standard or are in a similar vein:

• semistandard (standard with semicolons)
• happiness (standard with semicolons and tabs)
• doublestandard (standard with 2 semicolons 💃)
• uber-standard (standard forked by the Uber team)
• eslint-config-airbnb (an eslint plugin based on Airbnb's styleguide)

I've hooked it up to npm scripts because I was hoping on travis to run npm test on our behalf on CI, but it looks like it relies on grunt to do it instead. I'll update it.

661776a is the monster commit where I run standard-format on every file, and complete it with a few manual modifications that it's not smart enough to do by itself. Stuff I've had to do is:

• add path.join() calls where __dirname + '/foo' was being used
• split var a,\nb,\nc declarations into multiple ones
• add /* global MyLib */ calls to make it clear that stuff like jQuery or GOVUK is being read from the global context

Hope I didn't waffle too much, I can update the commit/PR messages with more info if any of this is useful to have for posterity.

[joelanman]

thanks! When does the linter run? Or do you have to run it manually?

[tvararu]

@joelanman my hope is to update this so that it runs as part of CI, and combined with text editor plugins that's usually enough for projects I've used it in with other developers. Additionally we could also run the test suite on a pre-push hook, to save a bit of time and get less red ❌'s on pull requests. (I can add that fairly easily if everyone likes the idea)

[tvararu]

TODO:

• Hook npm test into CI
• Add documentation about manually running npm run lint or adding editor plugins

[tvararu]

@joelanman I've written some documentation here: 123082a

This PR is good to go once #223 and #233 are merged.

[NickColley]

@tvararu like the addition in the docs, do you think we could briefly add why we're now linting and why standard being so opinionated is a good thing?

[tvararu]

@NickColley I've updated the latest commit, added two more blurbs on "Why lint?" and "Why standard?" 👌

[robinwhittleton]

Spoke about this in #frontend. The specific problem this solves is standardising the JS that goes into govuk_prototype_kit, it’s not necessarily being suggested for actual prototypes (although of course people using the kit are free to adopt for their own code if they wish).

[dsingleton]

Hugely pro some kind of standardised linting across our projects. 👍

As discussed in the FE meeting, standard seems like it might be a good default. Happy to try it for a while on this repo, see how it feels, and roll it out to more repos if it works for us.

[tvararu]

Thanks for the support @dsingleton!

I recall @edwardhorsford mentioning that he'd prefer not to link to the Linting doc from the main page of the README, because it's too technical. There are also a few other things I recall:

• Update README, remove link to "Linting", link from the CONTRIBUTING file instead
• Update Linting.md, expand on "Why standard" section (I don't remember exactly what though, @dsingleton to provide guidance)
• Update Linting.md, provide clear instructions for how to add linting to two popular editors (Sublime Text and Atom) and add a link to the official docs for other editors

Anybody subscribed to this thread feel free to edit this comment with things I've forgotten.

[gemmaleigh]

If you'd like another project to try this out on, you are more than welcome to do the same for GOV.UK elements!

Aside from the JS from the govuk_frontend_toolkit, some of the govuk_prototype_kit JS has been directly copied from govuk_elements.

If we have JS that can be of benefit to more than one project, it makes sense to move it to govuk_frontend_toolkit.

An example of this is the show/hide js, which will replace what is exists in application.js.

(This is just a FYI, it won't affect this PR - but will mean we can simplify application.js in the future!)

[dsingleton]

Update Linting.md, expand on "Why standard" section (I don't remember exactly what though, @dsingleton to provide guidance)

My train of thought around this was: Linting rules can be a contentious subject, and a personal preference. We should document why we chose a particular set of rules over others - why standard was a good fit for us (rather than why it's good in general). If we make sure people can understand the reasons behind that choice we're less likely to have rehash discussions over the choice.

Hopefully that helps?

[joelanman]

Happy to merge this if others are?

[tvararu]

@joelanman yes please! ✨

[joelanman]

@tvararu can you rebase against master and I'll merge

[joelanman]

thanks everyone! 💃