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.

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

Conversation

tihonove
Copy link
Contributor

@tihonove tihonove commented Oct 8, 2018

Just a little bug. Fixes #1922. Fixes #1993

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.

Could you add the test cases from both linked issues?

lib/rules/jsx-sort-props.js Outdated Show resolved Hide resolved
@tihonove
Copy link
Contributor Author

tihonove commented Oct 8, 2018

Could you add the test cases from both linked issues?

Currently tests contains reduced cases. Do you propose create test cases with exatly same code from issues?

@ljharb
Copy link
Member

ljharb commented Oct 8, 2018

Yes, auto fixing is pretty subtle, and it allows for complete confidence that it avoids the regression. “Too many tests” is a good problem to have :-)

@tihonove
Copy link
Contributor Author

Fix commit message

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.

Thanks!

@ljharb ljharb added the bug label Oct 11, 2018
@ljharb ljharb merged commit c2ae5d3 into jsx-eslint:master Oct 11, 2018
@@ -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.

This was referenced Dec 28, 2018
This was referenced Jan 4, 2019
@ghost ghost mentioned this pull request Jan 12, 2019
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants