airbnb / airbnb/javascript

Section 3.5 (sorting shorthand properties first) can impair readability

Open
#2,465 5 comments 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
148k
Forks
26.6k
PR merge metrics
No merged PRs in 30d

Description

Hello 👋! My company (VotingWorks) is attempting to use the Airbnb JavaScript Style Guide and ran into a problem with Section 3.5, which says:

> Group your shorthand properties at the beginning of your object declaration.
>> Why? It’s easier to tell which properties are using the shorthand.

It is true that it's easier to tell which properties are using the shorthand if you do it this way. I surmise that a more general motivation for this rule is to improve the ability for a human reading the object expression to understand it, i.e. make it more readable. However, this rule makes it harder for humans to understand the code in some circumstances:

### TypeScript discriminated unions

Let's say you have some types like this:

```ts
interface HandMarkedPaperBallotPage {
type: 'hmpb'
precinctId: string
ballotStyleId: string
pageNumber: number
marks: readonly Mark[]
// …
}

interface BallotMarkingDeviceBallotPage {
type: 'bmd'
precinctId: string
ballotStyleId: string
votes: Record
// …
}

type PageInterpretation =
| HandMarkedPaperBallotPage
| BallotMarkingDeviceBallotPage
```

`PageInterpretation` is a discriminated union of two variants: `HandMarkedPaperBallotPage` and `BallotMarkingDeviceBallotPage`. When working with a `PageInterpretation`, the `type` property helps TypeScript _discriminate_ between the possible variants, leading to type-safe access for the properties that are specific to that variant:

```ts
if (page.type === 'hmpb') {
console.log('hand-marked paper had these marks:', page.marks)
} else {
console.log('ballot marking device recorded these votes:', page.votes)
}
```

When a developer writes an object expression of type `PageInterpretation` it is most common to type the discriminator first like so:

```ts
function interpretImage(imagePath: string): PageInterpretation {
// …
if (detectedBmdBallot) {
const precinctId = detectedBmdBallot.getPrecinctId()
// …
return {
type: 'bmd',
precinctId,
// …
}
}
}
```

There are two reasons for this:
1. it makes it immediately clear which variant type the object has.
2. it tells an IDE (like VS Code) which properties to help autocomplete when typing the code for the object.

**Without a leading discriminator (shows all properties for all variants):**
![image.png](https://zube.io/files/votingworks/5c0ebae865261fafc56dba1664398b19-image.png)

**With a leading discriminator (shows only the properties for the detected variant):**
![image.png](https://zube.io/files/votingworks/926cfa516db5d26009584122c0798edc-image.png)

### Domain-specified property order

In many domains the order of properties maps to the order of values from the domain. For example, dates and times:

```ts
DateTime.fromObject({
year,
month,
day: lastDayOfMonth && day > lastDayOfMonth ? lastDayOfMonth : day,
hour,
minute: name === 'minute' ? partValue : newValue.minute,
zone: newValue.zone,
})
```

Following rule 3.5 here would require that `hour` move above `day`, hurting readability. I could unnecessarily write it as `hour: hour` (violating the `object-shorthand` rule) or come up with a new name for `hour` so that it cannot be written as shorthand instead.

Similarly, the HTTP request/response cycle is another domain with a natural ordering of properties following the order of values in the actual content of a request or response:

```ts
fetchMock.patchOnce('/config/election', {
status: 400,
body,
})
```

Following rule 3.5 here would put `body` above `status`, again hurting readability.

### Arbitrary inconsistency in object property ordering

In many places, especially test files, it is common to build many of the same object type over and over. Requiring that the order of properties vary depending on which properties happen to be eligible for shorthand hurts readability and [maybe even performance in v8](https://v8.dev/blog/fast-properties).

## Why is this a problem?

You could rightfully point out that `eslint-config-airbnb` does not actually enforce this rule. I agree that for nearly everyone this is not a problem since they can simply choose to ignore this requirement in scenarios like I outlined above. However, VotingWorks is attempting to certify our voting system with the [VVSG 2.0](https://www.eac.gov/sites/default/files/TestingCertification/Voluntary_Voting_System_Guidelines_Version_2_0.pdf) federal standard defined by the EAC. That document has the following requirement:

> **2.1-C – Acceptable coding conventions**
> Application logic must adhere to a published, credible set of coding rules, conventions, or standards (called "coding conventions") that enhance the workmanship, security, integrity, testability, and maintainability of applications.
> …
> Coding conventions are considered to be **published** if they appear in a publicly available book, magazine, journal, or new media with analogous circulation and availability, or if they are publicly available on the Internet. This requirement attempts to clarify the “published, reviewed, and industry-accepted” language appearing in previous iterations of the VVSG, but the intent of the requirement is unchanged.
> Coding conventions are considered to be **credible** if at least two different organizations with no ties to the creator of the rules or to the manufacturer seeking conformity assessment, and which are not themselves voting equipment manufacturers, independently decided to adopt them and made active use of them at some point within the three years before conformity assessment was first sought. This requirement attempts to clarify the “published, reviewed, and industry-accepted” language appearing in previous iterations of the VVSG, but the intent of the requirement is unchanged.

The Airbnb JavaScript Style Guide is likely the best choice to meet such a requirement, and as I mentioned we intend to fully adopt it. However, we don't have the luxury of picking and choosing the parts that we consider to be good, so we would likely have to follow the whole thing to the letter. Section 3.5, as written, would make our codebase worse. Here's a pull request I created that updates the codebase to follow this requirement: https://github.com/votingworks/vxsuite/pull/778. I can't pick a single change that obviously makes it better, and I can pick many that make it worse. The examples above came from or were inspired by this PR.

Airbnb itself doesn't even follow this rule in its open source projects:
- [react-dates/CalendarMonth.jsx](https://github.com/airbnb/react-dates/blob/6c2cb61c5e876fbdac72e6efed49c0bd64f06d6b/src/components/CalendarMonth.jsx#L223-L237)
- [react-dates/DateRangePicker_spec.jsx](https://github.com/airbnb/react-dates/blob/6c2cb61c5e876fbdac72e6efed49c0bd64f06d6b/test/components/DateRangePicker_spec.jsx#L599)
- [react-dates/DayPickerSingleDateController.jsx](https://github.com/airbnb/react-dates/blob/6c2cb61c5e876fbdac72e6efed49c0bd64f06d6b/src/components/DayPickerSingleDateController.jsx#L201-L207)
- [react-dates/DayPicker.jsx](https://github.com/airbnb/react-dates/blob/6c2cb61c5e876fbdac72e6efed49c0bd64f06d6b/src/components/DayPicker.jsx#L218-L234)
- [ts-migrate/index.ts](https://github.com/airbnb/ts-migrate/blob/9beed0ec25a9bb348053cb8c5575802e2276c5c3/packages/ts-migrate-server/src/migrate/index.ts#L74-L81)
- [visx/DataProvider.ts](https://github.com/airbnb/visx/blob/59ded3f6f69b20bd5a3dd258f6af20fd0e39c8fb/packages/visx-xychart/src/providers/DataProvider.tsx#L87-L102)

## What should change?

I think this rule should either be scrapped entirely or modified to focus on the real aim: improving the ability to understand the object. While ordering properties with shorthand properties first _can_ improve readability, it should be counterbalanced against other concerns such as domain-specific property order or better enabling discriminated unions. If this rule is left in, it should make it clear that these or other concerns may override it.

Thanks for your time 🙏 I know this was a rather long issue. We complain because we care ❤️

Contributor guide

No contributing guide indexed for this repository

Research direction

Read Section 3.5 of the Airbnb JavaScript Style Guide alongside the TypeScript discriminated-union and domain-ordering examples in this issue. Review the linked VotingWorks pull request and the cited Airbnb project examples, then determine whether the guidance needs removal or explicit exceptions. Done means the published rule clearly reflects the maintainers’ chosen readability guidance.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, typescript
Domain
documentation
Issue type
Documentation
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.