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

react/jsx-sort-props + {ignoreCase: false} now ignores case when sorting props #2530

Closed
hawkrives opened this issue Dec 21, 2019 · 10 comments
Closed

Comments

@hawkrives
Copy link

I've found an unexpected(?) change that stems from #2391.

<Hello name="John" Number="2" />

With {ignoreCase: false}, which according to the docs should take case into account when sorting the props, the above example now expects name to be sorted before Number.

I know why it sorts the way it does: #2391 switched to using String#localeCompare, which does…

> 'name'.localeCompare('Number')
-1

Looking at the lib/rules/jsx-sort-props.js code, I'm not sure what changes would be required to accommodate both this issue and #2381, unfortunately.

So, the crux of this issue is: is this an unexpected change, or should I alter my expectations and update the documentation to reflect this behavior?

@ljharb
Copy link
Member

ljharb commented Dec 21, 2019

This definitely seems like an unexpected change.

@ljharb
Copy link
Member

ljharb commented Dec 21, 2019

Specifically, we should probably only use localeCompare when ignoreCase is true?

@jcarty
Copy link

jcarty commented Jan 23, 2020

@ljharb any news on this one?

@ljharb
Copy link
Member

ljharb commented Jan 23, 2020

No, "help wanted" means it's awaiting a PR.

@tanmoyopenroot
Copy link
Contributor

We can't depend on localeCompare as it is not reliable.
Example:

Screenshot 2020-01-28 at 12 30 03 PM

Therefore, we need to replace localeCompare from line and line.

@tanmoyopenroot
Copy link
Contributor

Hi @hawkrives ,
I have added a PR, Can you check the test cases?

@ljharb
Copy link
Member

ljharb commented Jan 28, 2020

@tanmoyopenroot we must depend on localeCompare whenever possible since english isn't the only language in the world.

@tanmoyopenroot
Copy link
Contributor

@ljharb But each character in a string in any language will be compared in Unicode order for comparison operator.
In Unicode everything is ordered alphabetically.

@ljharb
Copy link
Member

ljharb commented Jan 28, 2020

If that were true, there'd be no point in the localeCompare method existing.

@tanmoyopenroot
Copy link
Contributor

I will try to fix it using localeCompare .

ljharb added a commit to tanmoyopenroot/eslint-plugin-react that referenced this issue May 12, 2020
Fixes jsx-eslint#2381. Fixes jsx-eslint#2530.

Co-authored-by: tanmoyopenroot <tanmoy.openroot@gmail.com>
Co-authored-by: Jordan Harband <ljharb@gmail.com>
@ljharb ljharb closed this as completed in f94d851 May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants