Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

alexzherdev
Copy link
Contributor

Resolves #1940

alexzherdev
alexzherdev commented Sep 5, 2018
View reviewed changes
lib/util/comments.js
} else {
let currentToken = token;
do {
const previousToken = sourceCode.getTokenBefore(currentToken);
Copy link
Contributor Author

@alexzherdev alexzherdev Sep 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@alexzherdev alexzherdev Jan 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we backported the eslint API, would that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tests/lib/rules/sort-prop-types.js
@@ -1533,5 +1533,1330 @@ ruleTester.run('sort-prop-types', rule, {
' }',
'});'
].join('\n')
}, {
code: `
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestions for more test cases are welcome.

@alexzherdev alexzherdev force-pushed the 1940-sort-prop-types-comments branch from e23cc32 to 44fe90b Compare September 5, 2018 02:38
@alexzherdev 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
Copy link
Member

ljharb commented Dec 27, 2018

This could use a rebase.

@alexzherdev
Copy link
Contributor Author

Will look into that today 👍

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

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

@ljharb
Copy link
Member

ljharb commented Dec 28, 2018

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

ljharb
ljharb reviewed Dec 28, 2018
View reviewed changes
lib/rules/sort-prop-types.js
@@ -120,6 +122,90 @@ module.exports = {
return 0;
}

function getRelatedComments(node, nextNode) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this live in a shared utility?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

lib/util/comments.js
} else {
let currentToken = token;
do {
const previousToken = sourceCode.getTokenBefore(currentToken);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@amannn
Copy link

amannn commented Jan 30, 2019

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

@amannn amannn mentioned this pull request Jan 30, 2019
Closed
@alexzherdev
Copy link
Contributor Author

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

@amannn
Copy link

amannn commented Jan 30, 2019

@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
Copy link

DNR500 commented Jun 20, 2019 •
edited
Loading

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

@alexzherdev
Copy link
Contributor Author

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

@amannn
Copy link

amannn commented Jun 21, 2019

@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
Copy link
Member

ljharb commented Jun 21, 2019

@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
Copy link
Member

ljharb commented Nov 23, 2019

@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
Copy link
Member

ljharb commented May 9, 2020

ping @alexzherdev, any update?

@ljharb ljharb marked this pull request as draft August 16, 2021 22:09
@ljharb ljharb force-pushed the master branch 3 times, most recently from 58a3c8b to deb3090 Compare November 10, 2022 20:26
@ljharb ljharb force-pushed the master branch 3 times, most recently from 59af733 to 865ed16 Compare November 11, 2022 02:45
@ljharb ljharb force-pushed the master branch 4 times, most recently from 069314a to 181c68f Compare November 18, 2022 17:19
@ljharb
Copy link
Member

ljharb commented Nov 28, 2022

Closing in favor of #3471.

@ljharb ljharb closed this Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

Assignees
No one assigned
Labels
enhancement
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sort-prop-types fix to account for comments
4 participants
@alexzherdev @ljharb @amannn @DNR500