Section 3.5 (sorting shorthand properties first) can impair readability

[eventualbuddha]

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: votingworks/vxsuite#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 ❤️

[ljharb]

Until someone inside Airbnb informs me that the company uses a different convention, the guide won't be changing here.

As I recall, the goal is specifically to maximize shorthand usage. You can do this in one of your examples like this:

// rename other `day` and `minute` to something else
const day = lastDayOfMonth && day > lastDayOfMonth ? lastDayOfMonth : day
const minute = name === 'minute' ? partValue : newValue.minute;
const { zone } = newValue;
DateTime.fromObject({ year, month, day, hour, minute, zone });

At the moment, this styleguide assumes you're authoring JS, so your TS type requirements don't have a bearing on it; if in the future Airbnb publishes a TS styleguide, this seems like a compelling argument for that guide.

[eventualbuddha]

I know I proposed eliminating the rule altogether, but I'd settle for a softening of the language to make it less absolute. Something like this:

Group your shorthand properties at the beginning of your object declaration when it improves readability.

[ljharb]

Leaving parts of a style guide up to subjectivity entirely defeats the purpose of having one.

[eventualbuddha]

Leaving parts of a style guide up to subjectivity entirely defeats the purpose of having one.

I get where you're coming from with that, and in general I agree. However, it's a balance. A style guide should recognize when specifying an absolute does a disservice to those trying to follow it. Even this style guide leaves some things up to judgement, such as 13.4 Assign variables where you need them, but place them in a reasonable place.

On the other hand, if you and/or Airbnb think this rule really should be followed all the time, then I suggest it should be enforced by an ESLint rule. It was actually quite simple to write one. Here it is:

module.exports = {
  name: 'object-shorthand-grouping',
  meta: {
    docs: {
      description:
        'Group your shorthand properties at the beginning of your object declaration',
      category: 'Best Practices',
      recommended: 'error',
      suggestion: true,
      requiresTypeChecking: false,
    },
    messages: {
      groupProperty:
        'Shorthand properties must be grouped at the beginning of the object',
    },
    schema: [],
    type: 'problem',
  },
  defaultOptions: [],

  create(context) {
    return {
      ObjectExpression(node) {
        let hasSeenNonShorthand = false

        for (const property of node.properties) {
          if (property.type === 'Property') {
            if (property.shorthand) {
              if (hasSeenNonShorthand) {
                context.report({
                  node: property,
                  messageId: 'groupProperty',
                })
              }
            } else {
              hasSeenNonShorthand = true
            }
          }
        }
      },
    }
  },
}

I would bet that almost no codebase that uses this style guide that were to enable such a rule would see zero errors, nor would fixing the violations of this rule result in a meaningful improvement most of the time. I would further bet this is true of Airbnb's internal JavaScript projects as well as their open source ones.

I'll leave it at this for now and await someone inside Airbnb to consider my arguments. Until then, thank you @ljharb for looking at this and considering it, despite our disagreement on the subject.

[Ansvyager]

its really cool