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

Fix fixer for jsx-sort-comp in case of more than 10 props #2012

Merged
merged 1 commit into from
Oct 11, 2018
Merged

Fix fixer for jsx-sort-comp in case of more than 10 props #2012

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/rules/jsx-sort-props.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ const generateFixerFunction = (node, context, reservedList) => {
});
});

fixers.sort((a, b) => a.range[0] < b.range[0]);
fixers.sort((a, b) => b.range[0] - a.range[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Woah great fix here. @tihonove could you explain how this fix works?

Copy link
Member

Choose a reason for hiding this comment

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

Comparators are supposed to only return a negative, positive, or zero number - previously it returned a boolean here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah thanks, I wasn't really aware of that.
But then I just couldn't piece together what this had to do with the original issue of 10 or more props. Why 10? Is it exactly 10, or somewhere around that number?
For posterity - I tested and noticed that booleans do work on arrays of 10 items or less, and immediately break on longer arrays. Something clicked and I recalled that V8 uses a different algorithm on small arrays; turned out that according to the source 10 is the threshold for that.
Which I guess makes sense because their quicksort implementation uses all 3 possible comparison results, whereas insertion sort only requires 2, so a boolean value is enough.


const rangeStart = fixers[fixers.length - 1].range[0];
const rangeEnd = fixers[0].range[1];
Expand Down
95 changes: 95 additions & 0 deletions tests/lib/rules/jsx-sort-props.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,101 @@ ruleTester.run('jsx-sort-props', rule, {
`,
errors: 3
},
{
code: '<App b={2} c={3} d={4} e={5} f={6} g={7} h={8} i={9} j={10} k={11} a={1} />',
output: '<App a={1} b={2} c={3} d={4} e={5} f={6} g={7} h={8} i={9} j={10} k={11} />',
errors: 1
},
{
code: `<List
className={className}
onStageAnswer={onStageAnswer}
onCommitAnswer={onCommitAnswer}
isFocused={isFocused}
direction={direction}
allowMultipleSelection={allowMultipleSelection}
measureLongestChildNode={measureLongestChildNode}
layoutItemsSize={layoutItemsSize}
handleAppScroll={handleAppScroll}
isActive={isActive}
resetSelection={resetSelection}
onKeyboardChoiceHovered={onKeyboardChoiceHovered}
keyboardShortcutType
/>`,
output: `<List
allowMultipleSelection={allowMultipleSelection}
className={className}
direction={direction}
handleAppScroll={handleAppScroll}
isActive={isActive}
isFocused={isFocused}
keyboardShortcutType
layoutItemsSize={layoutItemsSize}
measureLongestChildNode={measureLongestChildNode}
onCommitAnswer={onCommitAnswer}
onKeyboardChoiceHovered={onKeyboardChoiceHovered}
onStageAnswer={onStageAnswer}
resetSelection={resetSelection}
/>`,
errors: 10
},
{
code: `<CreateNewJob
closed={false}
flagOptions={flagOptions}
jobHeight={300}
jobWidth={200}
campaign='Some Campaign name'
campaignStart={moment('2018-07-28 00:00:00')}
campaignFinish={moment('2018-09-01 00:00:00')}
jobNumber={'Job Number can be a String'}
jobTemplateOptions={jobTemplateOptions}
numberOfPages={30}
onChange={onChange}
onClose={onClose}
spreadSheetTemplateOptions={spreadSheetTemplateOptions}
stateMachineOptions={stateMachineOptions}
workflowTemplateOptions={workflowTemplateOptions}
workflowTemplateSteps={workflowTemplateSteps}
description='Some description for this job'

jobTemplate='1'
stateMachine='1'
flag='1'
spreadSheetTemplate='1'
workflowTemplate='1'
validation={validation}
onSubmit={onSubmit}
/>`,
output: `<CreateNewJob
campaign='Some Campaign name'
campaignFinish={moment('2018-09-01 00:00:00')}
campaignStart={moment('2018-07-28 00:00:00')}
closed={false}
description='Some description for this job'
flag='1'
flagOptions={flagOptions}
jobHeight={300}
jobNumber={'Job Number can be a String'}
jobTemplate='1'
jobTemplateOptions={jobTemplateOptions}
jobWidth={200}
numberOfPages={30}
onChange={onChange}
onClose={onClose}
onSubmit={onSubmit}
spreadSheetTemplate='1'

spreadSheetTemplateOptions={spreadSheetTemplateOptions}
stateMachine='1'
stateMachineOptions={stateMachineOptions}
validation={validation}
workflowTemplate='1'
workflowTemplateOptions={workflowTemplateOptions}
workflowTemplateSteps={workflowTemplateSteps}
/>`,
errors: 13
},
{
code: '<App key="key" b c="c" />',
errors: [expectedShorthandLastError],
Expand Down