Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update 'JavaScript coding style' page #936

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
168 changes: 110 additions & 58 deletions source/manuals/programming-languages/js.html.md.erb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
title: JavaScript coding style
last_reviewed_on: 2023-01-27
last_reviewed_on: 2024-09-13
review_in: 12 months
owner_slack: '#frontend'
---
Expand All @@ -24,113 +24,165 @@ owner_slack: '#frontend'

## Linting

We follow [standardjs](https://standardjs.com/), an opinionated JavaScript linter. In cases where [standard conflicts with a service's browser support](#when-to-use-standardx) we use [standardx][].
As a base, we follow conventions established by the [standardjs][] project.
They allow us to be consistent about how we write code while reducing the time spend debating which linting rules to pick.

All JavaScript files follow its conventions, and it typically runs on CI to ensure that new pull requests are in line with them.
Depending on their needs, projects may complement this initial set of rules with extra linting, for example:

[standardx]: https://github.com/standard/standardx
- other [ESLint][] plugins, like [es-x][] to check API compatibility with browsers your project supports
- [Prettier][] to enforce further code formatting conventions

You should be cautious to only amend the initial set of rules to resolve compatibility issues, and not as a means to adjust rules to individual preferences.

**Why**: Linting ensures consistency in the codebase and picks up on well-known issues. Using an opinionated set of rules allows us to limit time spend picking rules, focusing instead on getting consistency, which is more important.

[standardjs]: https://standardjs.com/
[ESLint]: https://eslint.org/
[es-x]: https://github.com/eslint-community/eslint-plugin-es-x
[Prettier]: https://prettier.io/docs/en/cli.html

### Tools

### Running standard manually
#### StandardJS's command line interface

To check the whole codebase, run:
If you're not looking to use make any amends to the StandardJS conventions, you can use [StandardJS' `standard` command line interface][standard-cli] to lint files in your repository without extra set up.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the word 'use' might be left over from a previous edit:

Suggested change
If you're not looking to use make any amends to the StandardJS conventions, you can use [StandardJS' `standard` command line interface][standard-cli] to lint files in your repository without extra set up.
If you're not looking to make any amends to the StandardJS conventions, you can use [StandardJS' `standard` command line interface][standard-cli] to lint files in your repository without extra set up.


```bash
```
npx standard
```
### Running standard in your editor

Easier than running standard manually is to install it as a plugin in your editor. This way, it will run automatically while you work, catching errors as they happen on a per-file basis.

There are [official guides for most of the popular editors](https://standardjs.com/index.html#are-there-text-editor-plugins).
[standard-cli]: https://standardjs.com/#usage

### When to use standardx
#### standardx

You should use [standardx][] when the standard's rule set conflicts with the browsers your project supports. For example, the [no-var](https://eslint.org/docs/rules/no-var) rule in standard - which prefers `let` or `const` over `var` - prevents JavaScript usage in versions of Internet Explorer < 11. Adopting this rule would mean explicitly breaking JavaScript for those browsers.
If the StandardJS's rule set conflicts with the browsers your project supports, you can use [standardx][] to amend which rules are running.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be either the StandardJS rule set or StandardJS's rule set:

Suggested change
If the StandardJS's rule set conflicts with the browsers your project supports, you can use [standardx][] to amend which rules are running.
If the StandardJS rule set conflicts with the browsers your project supports, you can use [standardx][] to amend which rules are running.


Once installed you can then override standard rules with an [`.eslintrc`][eslintrc] file or an `eslintConfig` entry in package.json ([example][govuk-warnings]).

You should be cautious to only make use of standardx to resolve compatibility issues and not as a means to adjust rules to individual preferences.
[standardx]: https://github.com/standard/standardx
[eslintrc]: https://eslint.org/docs/v8.x/use/configure/configuration-files
[govuk-warnings]: https://github.com/alphagov/govuk_publishing_components/commit/ea7f0becc76f73780b6cb33701bea9e58f15f91a

Note: you may find that using standardx complicates integration with text editors.
#### ESLint

[eslintrc]: https://eslint.org/docs/user-guide/configuring
[govuk-warnings]: https://github.com/alphagov/govuk_publishing_components/commit/ea7f0becc76f73780b6cb33701bea9e58f15f91a
ESLint is the most widely use JavaScript linter, and actually what StandardJS uses under the hood.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small typo:

Suggested change
ESLint is the most widely use JavaScript linter, and actually what StandardJS uses under the hood.
ESLint is the most widely used JavaScript linter, and actually what StandardJS uses under the hood.

Using it directly allows you to benefit from other plugins in the ESLint ecosystem to complement standard conventions,
and keep up to date with newer rules, for example related to newer language features.

Standard can be integrated by adding the `eslint-config-standard` to your ESLint configuration.

When adding extra ESLint plugins, most come with a `recommended` configuration that's worth using as a starter, rather than deciding on each rule individually. You can then add or remove rules as needs arise during the life of your project. In that area, automatically fixable rules are especially cheap to try out, as the tools will take care of updating your code for you.

#### Prettier

Prettier's only preoccupation is with only with [code formatting, not code quality][prettier-comparison].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is with only with - should this be is with?

It can be used as a complement to ESLint for further automated formatting, with much more advanced decisions in terms of indentation, spaces, line breaks,...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line ends with ,... is that meant to be there? If it's markdown apologies, not familiar with it.

It runs as a separate command (`npx prettier`) and the []`eslint-config-prettier`][prettier-linters] ensures there'll be no conflicts between the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should

[]`eslint-config-prettier`][prettier-linters]

be

[][`eslint-config-prettier`][prettier-linters]

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be:

Suggested change
It runs as a separate command (`npx prettier`) and the []`eslint-config-prettier`][prettier-linters] ensures there'll be no conflicts between the
It runs as a separate command (`npx prettier`) and [`eslint-config-prettier`][prettier-linters] ensures there'll be no conflicts between the

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence seems incomplete? ...ensures there'll be no conflicts between the


[prettier-comparison]: https://prettier.io/docs/en/comparison
[prettier-linters]: https://prettier.io/docs/en/integrating-with-linters

### When to run linting

#### On CI

Running standard in CI ensures that all pull requests meet our code conventions before getting merged on the `main` branch.
You should at least have this set up on your project.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: I'd maybe reword this as You should have this configured as part of your project.


#### Through pre-commit Git hooks

Waiting for CI to know if the code follows the convention can take a bit of time.
A pre-commit Git hook allows to get quicker feedback, directly on developers' machines.
Errors that are automatically fixable can be fixed at that stage without human intervention, as well,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Errors that are automatically fixable can be fixed at that stage without human intervention, reducing the effort of linting for developers.

reducing the effort of linting for developers.

### Why standard?
Tools like [Husky][] and [lint-staged][] can help consistently run linting before commit by respectively:
- setting up the hooks when dependencies get installed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: will a list render correctly like this in markdown without a line break between it and the preceding paragraph? (I can't check the rendered version for some reason)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't render as a list on my machine, so I think it does need the blank line

- running linting on the files staged for commit and adding any fixes to the current commit

Linting rules can be a contentious subject, and a lot of them are down to personal preference. The core idea of standard is to be opinionated and limit the amount of initial bikeshedding discussions around which linting rules to pick, because in the end, it's not as important which rules you pick as it is to just be consistent about it. This is why we chose standard: because we want to be consistent about how we write code, but do not want to spend unnecessary time picking different rules (which all have valid points).
[Husky]: https://typicode.github.io/husky/
[lint-staged]: https://www.npmjs.com/package/lint-staged

The standard docs have a [complete list of rules and some reasoning behind them](https://standardjs.com/rules.html).
#### In editors

Standard is also [widely used (warning: large file)](https://github.com/feross/standard-packages/blob/master/all.json) (which means community familiarity) and has a [good ecosystem of plugins](https://standardjs.com/awesome.html).
To get even quicker feedback, editor plugins can highlight issues while editing files.
They also allow to fix automatically fixable errors on save, saving further fixing effort.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: They can correct automatically fixable errors on save, saving further development effort.


If we decide to move away from it, standard is effectively just a preconfigured bundle of eslint, so it can easily be replaced by switching to a generic `.eslintrc` setup.
Each of the tools previously listed has plugin to help integrate with editors:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be plugins?


- [StandardJS editor plugins](https://standardjs.com/#are-there-text-editor-plugins)
- [ESLint editor plugins](https://eslint.org/docs/latest/use/integrations#editors)
- [Prettier editor plugins](https://prettier.io/docs/en/editors)

## Whitespace

Use soft tabs with a two space indent.

If you're using [Prettier](#prettier), this will be set up for you. Otherwise, you may want to configure a [`.editorconfig` file][editorconfig] accordingly.

**Why:** This follows the conventions used within our other projects.

## Naming conventions
[editorconfig]: https://editorconfig.org/

* Avoid single letter names. Be descriptive with your naming.
## Naming conventions

```javascript
// Bad
var n = 'thing'
function q () { ... }
Follow the following conventions when naming symbols in your JavaScript code.

// Good
var name = 'thing'
function query () { ... }
```
**Why:** This lets future developers know how to interact with objects and sets the appropriate affordances. It also follows the conventions of the standard library.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: is there an alternative word we could use to affordances? I had to look up what it means, maybe we could use something more commonly used?


**Why:** Descriptive names help future developers pick up parts of the code
faster without having to read it all.
### Variables, functions and parameters

* Use camelCase when naming objects, functions, and instances.
Use [camelCase](http://eslint.org/docs/rules/camelcase) when naming variables, functions and parameters.

```javascript
// Bad
var this_is_my_object = {}
var THISIsMyVariable = 'thing'
function ThisIsMyFunction() { ... }
const this_is_my_object = {}
const THISIsMyVariable = 'thing'
function ThisIsMyFunction(this_is_a_param) { ... }

// Good
var thisIsMyObject = {}
var thisIsMyVariable = 'thing'
function thisIsMyFunction() { ... }
const thisIsMyObject = {}
const thisIsMyVariable = 'thing'
function thisIsMyFunction(thisIsAParam) { ... }
```

* Use PascalCase when naming constructors or classes.
### Classes and constructors

Use PascalCase when naming classes or constructors.

```javascript
// Bad
function user (options) {
class user {
constructor(options) {
this.name = options.name
}
var Bob = new user({
function profile(options) {
this.user = options.user
}
const Bob = new user({
name: 'Bob Parr'
})
const BobProfile = new profile({
user: Bob
})

// Good
function User(options) {
this.name = options.name
class User {
constructor(options) {
this.name = options.name
}
}
var bob = new User({
function Profile(options) {
this.user = options.user
}
const bob = new User({
name: 'Bob Parr'
})
const bobProfile = new Profile({
user: bob
})
```

**Why:** This lets future developers know how to interact with objects and sets the appropriate affordances. It also follows the conventions of the standard library.

## CoffeeScript

Do not use CoffeeScript.

**Why:** It's an extra abstraction and introduces another language for developers to learn. Using JavaScript gives us guaranteed performance characteristics and more well known support paths.

## HTML class hooks

When attaching JavaScript to the DOM use a `.js-` prefix for the HTML classes.
Expand Down Expand Up @@ -192,12 +244,12 @@ function myModule ($element) {
function submitThing () { ... }
function getArgumentsForThing () { ... }

$element.click(submitThing)
$element.addEventListener('click', submitThing)
}

// Good
function MyModule ($element) {
$element.click($.bind(this.submitThing, this))
$element.addEventListener('click', this.submitThing.bind(this))
}
MyModule.prototype.showThing = function () { ... }
MyModule.prototype.hideThing = function () { ... }
Expand All @@ -211,7 +263,7 @@ GOVUK.myModule = {
submitThing: function () { ... },
getArgumentsForThing: function () { ... },
init: function ($element) {
$element.click(GOVUK.myModule.submitThing)
$element.addEventListener('click', this.submitThing.bind(this))
}
}
```
Expand Down