fix(jsx-sort-props): enforce prop sort when reservedFirst is enabled #1340
Conversation
If reservedFirst is enabled and noSortAlphabetically is off, then alphabetical order is only enforced in the reserved props but not in the other props. * Update generateFixerFunction to accept the reservedList. * Rename alphabeticalCompare to be propNameCompare. Accept a reservedFirst and reservedList parameters in order to be able to compare prop names according to the configuration.
tests/lib/rules/jsx-sort-props.js
Outdated
| code: '<App dangerouslySetInnerHTML={{__html: "EPR"}} key={2} b />', | ||
| options: reservedFirstAsBooleanArgs, | ||
| errors: [expectedReservedFirstError] | ||
| errors: [expectedReservedFirstError, expectedError] |
There was a problem hiding this comment.
why would this suddenly start giving two errors?
There was a problem hiding this comment.
I think that after the changes this is triggering the expectedError as the || (!previousIsReserved && !currentIsReserved) check and it is true when evaluating dangerouslySetInnerHTML and b.
There was a problem hiding this comment.
We should try to avoid generating the extra error.
|
I updated the tests with the expected output, but I the fix is not taking into account the reserved words: Expected: I'm gonna try to see if I can add a fix for this. |
tests/lib/rules/jsx-sort-props.js
Outdated
| { | ||
| code: '<App b a />', | ||
| options: reservedFirstAsBooleanArgs, | ||
| errors: [expectedError] |
tests/lib/rules/jsx-sort-props.js
Outdated
| @@ -202,9 +202,19 @@ ruleTester.run('jsx-sort-props', rule, { | |||
| errors: [expectedError] | |||
| }, | |||
| { | |||
| code: '<App key={2} b a />', | |||
| options: reservedFirstAsBooleanArgs, | |||
| errors: [expectedError] | |||
There was a problem hiding this comment.
Please add output: <App key={2} a b /> (assuming that's the expected output)
|
Sorry I took so long to take a look at the change requests.
|
|
Definitely wanted, thanks :-) |
tests/lib/rules/jsx-sort-props.js
Outdated
| code: '<App dangerouslySetInnerHTML={{__html: "EPR"}} key={2} b />', | ||
| options: reservedFirstAsBooleanArgs, | ||
| output: '<App key={2} b dangerouslySetInnerHTML={{__html: "EPR"}} />', |
There was a problem hiding this comment.
hmm - with reservedFirst tho, the expected output should be <App dangerouslySetInnerHTML={{__html: "EPR"}} key={2} b />, no?
There was a problem hiding this comment.
I think dangerouslySetInnerHTML is not a reserved prop name for this component. I think it is only considered reserved for DOM components and not react components. This code is deleting dangerouslySetInnerHTML from the reserved list for the cases where the component is not a DOM component.
There was a problem hiding this comment.
ahhhh true! good call :-) could you add an "e" prop as well, so it's clear that it's being sorted as a regular property?
|
Planning to merge this soon. |
|
Any idea on when the merge will happen @ljharb? |
If reservedFirst is enabled and noSortAlphabetically is off, then alphabetical order is only enforced in the reserved props but not in the other props. This PR fixes this.
If existing behavior is the desired, please ignore this PR.
Fixes #1320