Add fixer for jsx/sort-props #1273
Conversation
lib/rules/jsx-sort-props.js
Outdated
| b = b.toLowerCase(); | ||
| } | ||
|
|
||
| if (a < b) { |
There was a problem hiding this comment.
if they're both strings, this can be return a.localeCompare(b);
lib/rules/jsx-sort-props.js
Outdated
|
|
||
| // Sort props according to the context. Only supports ignoreCase. | ||
| sortedAttributes.sort(function(a, b) { | ||
| // Put spread attributes at the end. |
There was a problem hiding this comment.
autofixers are supposed to always be safe; it's not safe to automatically put spread props on the end because that might change the override order.
In other words, any element that has spread props is simply not safely autofixable - so if we still want to autofix situations without spread props, then we shouldn't be making any changes to elements with spread props.
tests/lib/rules/jsx-sort-props.js
Outdated
| output: '<App a b {...this.props} />;' | ||
| }, | ||
| { | ||
| code: '<App c {...this.props} b a />;', |
There was a problem hiding this comment.
for example, this might be totally valid, because this.props might contain b and/or a, and the dev might want to override their values.
|
Thanks for the feedback. That's a good point I never considered. I've made some changes that pass the tests and ignore the spread attributes. |
tests/lib/rules/jsx-sort-props.js
Outdated
| errors: 2 | ||
| }, | ||
| { | ||
| code: '<App d="d" b="b" {...this.props} c="a" a="c" />;', | ||
| output: '<App a="c" b="b" c="a" d="d" {...this.props} />;', | ||
| output: '<App a="c" b="b" {...this.props} c="a" d="d" />;', |
There was a problem hiding this comment.
sorry if i wasn't clear - i'm afraid this is also unsafe. Any element with a spread prop can not be touched in any way that would alter overrides of the spread object. So, in <App a="c" b="b" {...this.props} c="a" d="d" />, you could sort the list of [a, b], and the list of [c, d] - but a and b must always remain before ...this.props, and c and c must always remain after it.
Only a human has the knowledge to safely alter this, and that can only be done manually.
|
Thanks for the quick feedback. I'm determined to get this in, let me know what you think. I changed it to consider the subgroups. |
There was a problem hiding this comment.
This particular rule has so many configurations, and I didn't bother to handle all of them although it wouldn't be hard to do so. I may do that in a follow-up PR.
It would be really great to get more coverage on the fixing. I'm mostly concerned about someone with a configuration where fixing may increase the number of warnings.
There was a problem hiding this comment.
@Overload119 i've rebased this for you, and some tests seem to be failing. Let me know when you've fixed it, and I'll rereview and merge.
lib/rules/jsx-sort-props.js
Outdated
| var attributes = node.attributes.slice(0); | ||
| var configuration = context.options[0] || {}; | ||
| var ignoreCase = configuration.ignoreCase || false; | ||
| const generateFixerFunction = (node, context) => { |
There was a problem hiding this comment.
(fwiw i think this was fine to leave as a normal function)
Hi all.
I've been meaning to dabble with eslint for a while, and this was a small project I wanted to take on. We're currently using this linter on our codebase, and I thought it would be great to have a fixer for this one.
This particular rule has so many configurations, and I didn't bother to handle all of them although it wouldn't be hard to do so. I may do that in a follow-up PR.
I tested using the following:
I also added an additional, multi-line test.