Skip to content

Add sortShapeProp option to sort-prop-types rule #1481

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.

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

Merged
merged 2 commits into from
Oct 30, 2017

Conversation

jomasti
Copy link
Contributor

@jomasti jomasti commented Oct 16, 2017

This allows the rule to enforce the sorting rules in a shape prop object.

I think it makes more sense to happen by default, but I expect it would be better to wait until the next major version for that. This PR should allow the rule to satisfy the request in #1476.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM overall.

In no way do I think that it should ever become the default; sorting prop names doesn't necessarily mean shape props should be sorted.

Separately, please add test cases that ensures that shape properties are not sorted such that they cross a spread boundary. Example that should not have an error:

Component.propTypes = {
  a: PropTypes.shape({
    d: PropTypes.any,
    ...someObject,
    a: PropTypes.any
  })
};

}],

// Invalid code, should not be validated
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for this comment?

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 think I saw the comment as an intended marking of where the invalid test cases began and that it was in the wrong place. Looking back over it, the comment was actually in the right spot. I'll rebase to put this back.

@jomasti jomasti force-pushed the issue-1476 branch 2 times, most recently from b5210fe to 7bbf797 Compare October 29, 2017 17:29
@ljharb
Copy link
Member

ljharb commented Oct 29, 2017

Please rebase this one more time; tests should pass now :-)

This allows the rule to enforce the sorting rules in a shape prop
object.
@ljharb ljharb merged commit aad4a37 into jsx-eslint:master Oct 30, 2017
@jomasti jomasti deleted the issue-1476 branch October 30, 2017 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants