search

vercel / style-guide

Vercel's engineering style guide
Mozilla Public License 2.0
1.18k stars 29 forks source link

Proposal to remove the jsx-sort-props eslint rule #79

Closed dferber90 closed 3 months ago

dferber90 commented 7 months ago

Description

Problem

The jsx-sort-props eslint rule in our style guide enforces alphabetical sorting of props in JSX components. While consistency in code style is good, this specific rule does not seem worth the burden it introduces.

Rationale

  1. Subjectivity: The alphabetical sorting of props can be subjective and may not necessarily contribute to improved code readability or maintainability. Developers may find it cumbersome to maintain this order, especially when dealing with a large number of props.

  2. Limited Benefit: The perceived benefits of enforcing alphabetical sorting might not outweigh the drawbacks, leading to a potential decrease in developer productivity without a significant improvement in code quality. I would argue a code base with sorted props produces the exact same value as a code base with unsorted props.

  3. Flexibility: Codebases and components vary, and a one-size-fits-all approach may not be suitable. Certain components or scenarios might benefit more from a different prop ordering strategy.

  4. Lost time when editing: When copying example code from docs or component libraries the props will often not be sorted. In those cases it is first necessary to manually reorder the props, or to force the editor to reorder them. Some developers will have setups which sort props automatically, but others will not have those which leads to manual labor for reordering or lost time for setting up such automations.

  5. Lost time when publishing: At times developers will not notice the ESLint issue in their editor, push the commit, and then only find out through CI about incorrectly sorted props. They then need to investigate why CI failed, only to see it was an unsorted prop. Finding out about this issue can already take upwards of 10 minutes, in which they might have already switched context. They then need to go back to the editor, check out the previous branch, fix the issue, commit again, push again, and wait for CI again. This whole cycle can easily take upwards of 30 minutes. This is a huge price to pay for the limited benefit it provides

  6. Unnecessary diff when props are renamed: Whenever you rename a prop, it could be that the new name no longer matches the expected sorting, so the prop also needs to be moved. This leads to a more confusing diff as a simple rename results in a deletion & addition.

Renaming a prop with sorted props leads to an addition and deletion

image

Renaming a prop with unsorted props

image

Concerns

When we get a mix of developers who have automatic sorting of props on save set up in their editor, and those developer save a file which contains unordered props, then this will lead to unnecessary diffs in pull requests, which is distracting during code reviews and gets in the way of git blame.

Proposal

I propose the removal of the jsx-sort-props rule from our style guide. This decision is based on the aforementioned concerns and the belief that the value of having sorted props does not outweigh the time that is lost when CI rejects PRs due to unsorted props.

vercel-release-bot commented 3 months ago

:tada: This issue has been resolved in version 5.3.0-canary.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

vercel-release-bot commented 3 months ago

:tada: This issue has been resolved in version 6.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: