Add fixer for sort-prop-types #1891
Conversation
There was a problem hiding this comment.
Please add test cases that use spreading, to ensure that nothing is ever sorted across a spread boundary.
In other words, d, b, ...c, e, a should become b, d, ...c, a, e, and nothing should ever change position with respect to c (due to overriding).
(Note that this kind of problem is why autofixers for any sorting of anything is prohibitively difficult)
|
@ljharb Yeah, I noticed earlier that this is an issue. I added an additional testcase for this and adapted the fixing code to make sure this doesn't happen. I also ended up adapting the fixing code to also work with the additional options as well. |
lib/rules/sort-prop-types.js
Outdated
| @@ -83,6 +85,78 @@ module.exports = { | |||
| return; | |||
| } | |||
|
|
|||
| function fix(fixer) { | |||
| function sortInSource (allNodes, source) { | |||
There was a problem hiding this comment.
no space after the function name
lib/rules/sort-prop-types.js
Outdated
| }, [[]]); | ||
|
|
||
| nodeGroups.forEach(nodes => { | ||
| const sortedAttributes = nodes.slice(0).sort((a, b) => { |
There was a problem hiding this comment.
This sorting function should probably be defined at file level, since it’s pure?
lib/rules/sort-prop-types.js
Outdated
| ); | ||
| sortedAttrText = attrSource.slice(sortedAttr.range[0], sortedAttr.range[1]); | ||
| } | ||
| source = `${source.substr(0, attr.range[0])}${sortedAttrText}${source.substr(attr.range[1])}`; |
There was a problem hiding this comment.
please use slice over substring/substr
|
@ljharb Thanks for the review, I incorporated your feedback. |
lib/rules/sort-prop-types.js
Outdated
| let sortedAttrText = sourceCode.getText(sortedAttr); | ||
| if (sortShapeProp && isShapeProp(sortedAttr.value)) { | ||
| const attrSource = sortInSource( | ||
| sortedAttr.value.arguments[0].properties, |
There was a problem hiding this comment.
does isShapeProp ensure that the value has a non-empty arguments? What about PropTypes.shape()?
There was a problem hiding this comment.
Good point, I added test cases for this. Also there was already a bug that cause the linting to fail on PropTypes.shape(). I included this fix: #1842
lib/rules/sort-prop-types.js
Outdated
| }, [[]]); | ||
|
|
||
| nodeGroups.forEach(nodes => { | ||
| const sortedAttributes = nodes.slice(0).sort(sorter); |
lib/rules/sort-prop-types.js
Outdated
|
|
||
| const rangeStart = declarations[0].range[0]; | ||
| const rangeEnd = declarations[declarations.length - 1].range[1]; | ||
| return fixer.replaceTextRange([rangeStart, rangeEnd], source.substr(rangeStart, rangeEnd - rangeStart)); |
|
@ljharb Good catch with |
This adds adds a fixer for the
react/sort-prop-typesrule.It fixes ordering issues including the
sortShapePropand theignoreCaseoption.It doesn't work yet forcallbacksLastorrequiredFirst, but that could be added later. (Right now those errors would just be ignored)