Section 3.5 (sorting shorthand properties first) can impair readability

Đọc Section 3.5 của Airbnb JavaScript Style Guide cùng với các ví dụ TypeScript về discriminated-union và domain-ordering trong issue này. Xem xét pull request VotingWorks được liên kết và các ví dụ dự án Airbnb được trích dẫn, sau đó xác định xem hướng dẫn có cần được gỡ bỏ hay cần các ngoại lệ rõ ràng. Công việc được coi là hoàn thành khi quy tắc đã công bố phản ánh rõ ràng hướng dẫn về khả năng đọc do các maintainer lựa chọn.

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:

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

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

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:

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:

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):

With a leading discriminator (shows only the properties for the detected variant):

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:

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:

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.

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 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
• react-dates/DateRangePicker_spec.jsx
• react-dates/DayPickerSingleDateController.jsx
• react-dates/DayPicker.jsx
• ts-migrate/index.ts
• visx/DataProvider.ts

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 ❤️