Sort destructured props

[willdurand]

This issue is a follow-up to some comments in mozilla/addons-frontend#5652. When upgrading to React Router v4, most of the props have been sorted but some might have been missed, and we did not have an ESLint rule for that. The ESLint rule has since been created: https://github.com/mozilla/eslint-plugin-amo#sort-destructured-props.

---

We (try to) enforce sorted destructured properties in function arguments and destructured assignements:

const foo = ({ a, b }) => {};

const { a, b } = this.props;

This rule allows us to always know where to insert a new property, thus avoiding unnecessary thinking. We follow the "natural sort order":

const { _underscoreProps, CapitalizedProps, aLowerCase, bLowerCase, ...otherProps } = props;

⚠️ Thanks to the ESLint rule, we do not have to do anything anymore (it is fixed automatically), except when a prop is used as a default value of another prop in the same destructuration:

const foo = ({
  userId = 123,
  aPropThatShouldBeListedFirst = userId,
}) = > {
};

The ESLint rule will probably try to re-order the list of props as follows:

const foo = ({
  aPropThatShouldBeListedFirst = userId,
  userId = 123,
}) = > {
};

But another ESLint rule will throw an error because userId is used before it is declared. In this case, it is best to disable the amo/sort-destructured-props props:

// eslint-disable-next-line amo/sort-destructured-props
const foo = ({
  userId = 123,
  aPropThatShouldBeListedFirst = userId,
}) = > {
};

[kumar303]

This rule allows us to always know where to insert a new property, thus avoiding unnecessary thinking.

On the contrary, this rule enforces unnecessary thinking in that the coder has to alphabetize the properties 😞

I would rather we didn't do this. Editors are powerful these days (finding variables is easy) so I'm not sure why we need it. However, I understand that people other than me prefer to see things alphabetized so if this new rule minimizes those comments in code reviews then I'm fine with it. Carry on.

[willdurand]

On the contrary, this rule enforces unnecessary thinking in that the coder has to alphabetize the properties 😞

I don't know where this comes from to be honest, but I got a comment every time I was not applying this rule on props, imported symbols, imports etc. (and, I started contributing on this project almost a year ago).

Since this was the only comment I got every time and Prettier could not help me fix it, I wrote a fixable rule to automate such changes, because you're right: that's not something people have to do.

There were basically two options to get rid of those comments, either a tool or we stop leaving such comments. I guess I bet on the wrong option 😋

[kumar303]

I wrote a fixable rule to automate such changes

Oh, it's fixable? That changes things. I am still waiting for someone to tell me why we need to alphabetize destructured variables but if they can convince me then I guess we can add another pre-PR script like yarn prettier-dev and/or editor integration. I agree that we shouldn't waste code review time fixing these things.

[willdurand]

Oh, it's fixable?

haha yes, I would have never done the changes myself 🤣

I am still waiting for someone to tell me why we need to alphabetize destructured variables but if they can convince me

We tend to sort everything for some reasons, I guess main reasons are: consistency and easier to find a specific "thing" (prop, react prop, variable, import, etc.). I tend to like the idea of having vars/props starting with an underscore first because those are vars we use for "dependency injection" usually (so I can skip them when I want to find the props I am supposed to pass in the production code). Nothing super fancy though, and I can understand the fear of maintenance burden.

we can add another pre-PR script like yarn prettier-dev and/or editor integration.

$ yarn eslint --fix

[willdurand]

So, I think we can close this issue and mark it as wontfix.

The eslint rule in https://github.com/mozilla/eslint-plugin-amo#sort-destructured-props is not enabled by default (i.e. not part of the recommended rules), but still there if we ever want to re-open this issue. FTR, the eslint rule automatically fixes the code, there is not need to manually sort the props.