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

Make sort locale independent #5

Closed
wants to merge 3 commits into from
Closed

Make sort locale independent #5

wants to merge 3 commits into from

Conversation

padinko
Copy link
Contributor

@padinko padinko commented May 5, 2021

this update will sort keys as native .sort() which is C.UTF-8 (POSIX) sort - not locale independent.

this will fix locale sorting problem in npm's package-lock.json npm/cli#2829

same problem was fixed in 2017 https://github.com/npm/npm/pull/17844/files but now sorting package.json and package-lock.json are again locale independend in npm 7

for me this is a big issue, because some of us in our company is working with english OS, some in slovak and both package.json and package-lock files are changing with every npm install

if you are on *nix system, you can test LC_ALL=sk node test.js and LC_ALL=en-US node test.js with node 14 (node < 14 dont have full-icu, so only en-US was supported) to see different output of this library for example ch is 1 character is slovak language and its between h and i, but in english it's 2 character, so it is between b and d

test.js:

const stringify = require('./index');
console.log(stringify({ b:1, d:1 ,ch:1 }));

result:

$ LC_ALL=sk node test.js
{
  "b": 1,
  "d": 1,
  "ch": 1
}

$ LC_ALL=en-US node test.js
{
  "b": 1,
  "ch": 1,
  "d": 1
}

I was trying to fix this by forcing en-US locale, but, C and en-US sort is not the same; for example - is after _ in en-US, but before in C.
so npm 7 again don't have same sorting as npm 6, when 2017 fix (above) was applied and was using native C sort

@padinko
Copy link
Contributor Author

padinko commented May 6, 2021

updated code by this comment npm/arborist#276 (comment)
so locale can be set from outside

@isaacs
Copy link
Owner

isaacs commented May 6, 2021

I think it's best to just force 'en' as the locale, rather than make it configurable. As long as we support node 12 and below, we can't actually guarantee support for any other locales anyway, and there are some benefits to having it be somewhat reasonable rather than case-sensitive, etc.

@isaacs isaacs closed this in 08736fa May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants