Kibana: [DISCUSS] [tslint] Get rid of object-literal-sort-keys rule

[chrisdavies]

I've been writing some TypeScript lately, and this rule is driving me mad.

Our style guide says to put shorthand properties before any long-hand properties:

export const thing = {
  achoo,
  choo,
  bach: someFunctionHere(),
};

But this rule makes me have to do this:

export const thing = {
  achoo,
  bach: someFunctionHere(),
  choo,
};

Further, I often like to group my properties by logical association, or by normal English flow of words, especially for bigger objects:

{
   name: 'Oscar',
   address: 'Sesame Street',
   city: 'New York',
   state: 'SC',
   zip: 12324,
}

But this rule would make me do this:

{
   address: 'Sesame Street',
   city: 'New York',
   name: 'Oscar',
   state: 'SC',
   zip: 12324,
}

OK, maybe that's not the best example, as you'd probably split addresses out into their own object, but you get the point. I think alphabetical sorting is not terribly useful in general, but other groupings are often useful.

Thumbs up / down if you agree / disagree.

[lukasolson]

Our JS style guide actually says to put shorthand variables first, so we either need to change that or get rid of this lint rule.

[chrisdavies]

@lukasolson You're right. It's not encapsulated in our eslint rules. It probably should be in both eslint and tslint.

[cjcenizal]

Can someone who is for this rule explain how it benefits them? I don’t feel strongly one way or the other but if this rule impedes teammates and doesn’t provide a huge benefit, then I think removing it is a low cost way to improve the DX.

[chrisdavies]

Updated the description to include the style guide per Lucas's comment.

[kobelb]

Can someone who is for this rule explain how it benefits them? I don’t feel strongly one way or the other but if this rule impedes teammates and doesn’t provide a huge benefit, then I think removing it is a low cost way to improve the DX.

I find that with time and enough fields being added "logical" ordering generally falls apart and I have generally resorted to alphabetical ordering. No longer requiring shorthand properties to be listed first reduces the amount of noise in a diff when switching from shorthand to longhand and vice-versa.

[bmcconaghy]

I like how easy to understand the alphabetical ordering is. With the other ordering, I have to know what type of property it is in order to find it in a large list.

[epixa]

I don't have a strong opinion about alphabetical ordering, but I do think we should defer to the recommended linting rules wherever possible. We'll have exceptions to the recommended rules, but I'd much prefer they be considered further down the road once we've used them for a bit rather than in the days after the recommended rules were introduced in the first place.

Unquestionably the recommended rules will not be ideal to some people, just like the prettier styles aren't necessarily everyone's favorite. The point of these things is largely to take those decisions out of our hands to help us avoid inconsistencies and churn on our styles over time.

We did make an exception so far with not prefixing interfaces with I, but this was a style rule that had direct ramifications on our public plugin APIs, which made nipping it in the bud pretty important.

[stacey-gammon]

If we do leave it in, is there any way to automatically fix it? Seems to take up a good bit of time when I'm trying to commit without errors to go fix it manually... but maybe I'm not aware of a special command (like prettier has the command) to auto fix it?

[epixa]

Does the --fix flag work for the eslint command? If so, then yes. And you can also set up your editor to run that automatically on files (which will do things like add the license header as well).

[chrisdavies]

@Stacey-Gammon I don't know what editor you use, but VS Code automatically fixes it. You can click on the little yellow lightbulb and say "Fix all errors". So, it's not too egregious a rule from that perspective.

[bmcconaghy]

@Stacey-Gammon also in vscode you can add this to your settings:

"eslint.autoFixOnSave": true,
 "eslint.options": { "configFile": ".eslintrc.js" },

[kobelb]

Complete side-note, but the auto fix on save stuff makes working with the license headers so much nicer!

[azasypkin]

I'd still keep this rule since "logical grouping" of properties sounds like a very subjective thing that works well only until another developer decides to change the code and they may have different perspective on what "logical grouping" is. It's quite common for large teams like we have.

Having stricter rules that are enforced automatically may also reduce unnecessary friction during code review. In case when "logical grouping" feels really important then maybe it's time to extract these properties into sub-object or something.

[rhoboat]

Agree with keeping the object-literal-sort-keys rule, and updating the style guide to remove preferring shorthand properties at the top.

[chrisdavies]

We have 9 in favor and 6 opposed. That's probably too close to warrant making a change. Instead, I'm closing this, and we'll keep the tslint rules. I've opened a separate PR which will update our JS style guide to no longer specify a preference for property ordering.

[mattapperson]

A note that this is not done with a --fix flag https://github.com/palantir/tslint/issues/2583

[stacey-gammon]

Can we re-open this discussion since this is not auto-fixed? Or at least it doesn't seem to be for me. The import order is auto fixed but not object literal order. Manually alphabetizing is painful.

[mattapperson]

@Stacey-Gammon worth noting that the prettier team looked into an auto-fix for this and found it was darn near impossible as it could and likely would cause bugs https://github.com/prettier/prettier/issues/1096

[epixa]

I've changed my vote in favor of this change. I don't think this could be auto-fixed without resulting in some pretty gross code, which leads me to believe that this will never be autofixable.

At this point, I've seen this rule criticized at least a half dozen times on slack, along with an email thread and this issue. One of the reasons we want to use recommended rules is to prevent constant time dumped into discussing issues exactly like this. We should keep the recommend rules as much as we can, but if there is mostly overwhelming support for changing a rule and the only people that vote against it are doing that on ideological grounds, then that's pretty damning for the rule in question.

[stacey-gammon]

Created https://github.com/elastic/kibana/pull/20274