[New] `sort-prop-types`: support comments on prop types #1973

alexzherdev · 2018-09-05T01:27:58Z

Resolves #1940

alexzherdev · 2018-09-05T01:30:56Z

lib/util/comments.js

+  } else {
+    let currentToken = token;
+    do {
+      const previousToken = sourceCode.getTokenBefore(currentToken);


Note: this code was extracted from jsx-curly-spacing and for some reason this line (or the next) returns undefined on ESLint 3.0.0 😕(our test suite on master fails against that version). Works well on 3.19.0.

Is this still failing? It'd be useful to figure out which version of 3.x this api was introduced in.

So it was originally introduced in 3.0.0. My branch has 182 test failures on that version (not all of them seem to be related to this api or caused by my changes though. Master has 127 failures). Between 3.15.0 and 3.16.0 it drops from 169 to 34 failures, removing a lot of errors related to tokens/comments handling. Although a few of the remaining errors are me using getLocFromIndex in this PR, which was only introduced in 3.17.0 😕 Not sure what to do about that, it would not be feasible/doable to backport, but also there are other tests that are failing on that version anyway... We need at least 3.18.0 for 0 test failures.

If we backported the eslint API, would that work?

Looks like both getLocFromIndex and getIndexFromLoc are relying heavily on lineStartIndices and lines in the SourceCode class that get populated once in the constructor. The ESLint docs explicitly say we can use lines (bottom of the section) but there's no word about lineStartIndices. Potentially we can just copy over those functions, but the bigger issue with getTokenBefore/getTokenAfter remains, and those don't seem possible to be backported.

Is there a way we could runtime-detect these APIs, and provide a reasonable degraded experience for older eslint users?

I'm skeptical (not that we could detect them, but that there is a reasonable experience beyond just not applying fixes), but let me look into that.

yeah that's what i meant - applying fewer fixes when they weren't good enough.

alexzherdev · 2018-09-05T01:31:19Z

tests/lib/rules/sort-prop-types.js

@@ -1533,5 +1533,1330 @@ ruleTester.run('sort-prop-types', rule, {
      '  }',
      '});'
    ].join('\n')
+  }, {
+    code: `


Suggestions for more test cases are welcome.

ljharb · 2018-12-27T21:23:50Z

This could use a rebase.

alexzherdev · 2018-12-27T21:24:58Z

Will look into that today 👍

alexzherdev · 2018-12-28T04:08:24Z

Hmm appveyor actually failed (node 5 choked on args spread somewhere inside eslint), but there's a green check here.

ljharb · 2018-12-28T05:27:16Z

@alexzherdev node 5 is an allowed failure; eslint 5 also requires node 6+ now, so it's fine.

ljharb · 2018-12-28T05:28:27Z

lib/rules/sort-prop-types.js

@@ -120,6 +122,90 @@ module.exports = {
      return 0;
    }

+    function getRelatedComments(node, nextNode) {


Should this live in a shared utility?

I don't mind that. Although I imagine the isSameLine bit in the output is pretty specific to this use case, I don't think it is generally useful to know whether the comment related to a thing is on the same line as the thing or not 🤔

ljharb · 2018-12-28T05:29:07Z

lib/util/comments.js

+  } else {
+    let currentToken = token;
+    do {
+      const previousToken = sourceCode.getTokenBefore(currentToken);


Is this still failing? It'd be useful to figure out which version of 3.x this api was introduced in.

amannn · 2019-01-30T11:53:55Z

Is there something missing for this PR to get merged? I also encountered the problem described in the referenced issue.

alexzherdev · 2019-01-30T16:09:03Z

@amannn I will be tackling #1973 (comment) within a few days

amannn · 2019-01-30T16:24:30Z

@alexzherdev That's great to hear! Thank you so much for your help!

It's really cool to have auto fixable jsx-sort-props and sort-prop-types. After that, there's only auto fixing for jsx-sort-default-props missing for me to go all-in on sorting props 🙂.

DNR500 · 2019-06-20T15:49:22Z

Any resolution on this PR yet? Would be super useful for users of storybook

alexzherdev · 2019-06-20T16:36:56Z

Haven’t had time to get back to it yet. The issue in #1973 (comment) is a tricky one to make sense of.

amannn · 2019-06-21T06:04:16Z

@alexzherdev Not sure if that's an option, but since even the test suite on master fails for that version, would it be an option to bump the peer dependency of eslint to 3.18.0?

Would be a shame if the support for that version would block helpful contributions like this PR. ESLint 3.0.0 was released on July 1, 2016 – that's three years ago. Even node.js LTS releases have a stop of maintenance after 3 years.

ljharb · 2019-06-21T06:20:25Z

@amannn no, that'd be a breaking change.

Surely there's a way we can handle eslint 3.0 - even if that means gracefully degrading to the behavior prior to this PR.

ljharb · 2019-11-23T00:31:56Z

@alexzherdev I'm going to disable the entire autofix behavior of this rule before the next release unless we can find a way to get this PR in; it'd be great if you had time to update this.

ljharb · 2020-05-09T22:39:59Z

ping @alexzherdev, any update?

ljharb · 2022-11-28T20:05:48Z

Closing in favor of #3471.

alexzherdev commented Sep 5, 2018

View reviewed changes

alexzherdev force-pushed the 1940-sort-prop-types-comments branch from e23cc32 to 44fe90b Compare September 5, 2018 02:38

alexzherdev force-pushed the 1940-sort-prop-types-comments branch 2 times, most recently from 5592386 to faf1952 Compare September 15, 2018 21:40

ljharb added the enhancement label Dec 27, 2018

[New] sort-prop-types: support comments on prop types

67b88a8

alexzherdev force-pushed the 1940-sort-prop-types-comments branch from faf1952 to 67b88a8 Compare December 28, 2018 02:04

ljharb reviewed Dec 28, 2018

View reviewed changes

amannn mentioned this pull request Jan 30, 2019

Sort React prop types molindo/eslint-config-molindo#4

Closed

ljharb added enhancement and removed enhancement labels Apr 12, 2019

ljharb marked this pull request as draft August 16, 2021 22:09

ljharb force-pushed the master branch from 0d1a62e to f7fe38f Compare July 11, 2022 15:02

ljharb force-pushed the master branch 3 times, most recently from 58a3c8b to deb3090 Compare November 10, 2022 20:26

ljharb force-pushed the master branch 3 times, most recently from 59af733 to 865ed16 Compare November 11, 2022 02:45

ljharb force-pushed the master branch 4 times, most recently from 069314a to 181c68f Compare November 18, 2022 17:19

ljharb closed this Nov 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New] `sort-prop-types`: support comments on prop types #1973

[New] `sort-prop-types`: support comments on prop types #1973

alexzherdev commented Sep 5, 2018

alexzherdev Sep 5, 2018 •

edited

ljharb Dec 28, 2018

alexzherdev Jan 12, 2019 •

edited

ljharb Jan 13, 2019

alexzherdev Jan 15, 2019

ljharb Jan 15, 2019

alexzherdev Jan 15, 2019

ljharb Jan 16, 2019

alexzherdev Sep 5, 2018

ljharb commented Dec 27, 2018

alexzherdev commented Dec 27, 2018

alexzherdev commented Dec 28, 2018

ljharb commented Dec 28, 2018

ljharb Dec 28, 2018

alexzherdev Jan 12, 2019

ljharb Dec 28, 2018

amannn commented Jan 30, 2019

alexzherdev commented Jan 30, 2019

amannn commented Jan 30, 2019

DNR500 commented Jun 20, 2019 •

edited

alexzherdev commented Jun 20, 2019

amannn commented Jun 21, 2019

ljharb commented Jun 21, 2019

ljharb commented Nov 23, 2019

ljharb commented May 9, 2020

ljharb commented Nov 28, 2022

[New] sort-prop-types: support comments on prop types #1973

[New] sort-prop-types: support comments on prop types #1973

Conversation

alexzherdev commented Sep 5, 2018

alexzherdev Sep 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexzherdev Jan 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Dec 27, 2018

alexzherdev commented Dec 27, 2018

alexzherdev commented Dec 28, 2018

ljharb commented Dec 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amannn commented Jan 30, 2019

alexzherdev commented Jan 30, 2019

amannn commented Jan 30, 2019

DNR500 commented Jun 20, 2019 • edited

alexzherdev commented Jun 20, 2019

amannn commented Jun 21, 2019

ljharb commented Jun 21, 2019

ljharb commented Nov 23, 2019

ljharb commented May 9, 2020

ljharb commented Nov 28, 2022

[New] `sort-prop-types`: support comments on prop types #1973

[New] `sort-prop-types`: support comments on prop types #1973

alexzherdev Sep 5, 2018 •

edited

alexzherdev Jan 12, 2019 •

edited

DNR500 commented Jun 20, 2019 •

edited