Skip to content

Commit

Permalink
Avoid locale-specific string sorting issues
Browse files Browse the repository at this point in the history
This adds the 'en' locale to all instances of String.localeCompare
within the CLI codebase.

Tests added for the cases where we're sorting arbitrary user-generated
data.  The tests rely on the fact that 'ch' sorts after 'd' in the
`'sk'` locale, but ahead of `'d'` in the `'en'` locale.  To ensure that
this is the default behavior if no locale is specified, `LC_ALL=sk` is
set in the test environment.

Other instances of `localeCompare` sort data that the cli controls, so
no tests were added.

Re: #2829
  • Loading branch information
isaacs committed May 7, 2021
1 parent a098aef commit 1a759f9
Show file tree
Hide file tree
Showing 24 changed files with 363 additions and 327 deletions.
4 changes: 2 additions & 2 deletions lib/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ class Config extends BaseCommand {
; Configs like \`//<hostname>/:_authToken\` are auth that is restricted
; to the registry host specified.
${data.split('\n').sort((a, b) => a.localeCompare(b)).join('\n').trim()}
${data.split('\n').sort((a, b) => a.localeCompare(b, 'en')).join('\n').trim()}
;;;;
; all available options shown below with default values
Expand Down Expand Up @@ -227,7 +227,7 @@ ${defData}
if (where === 'default' && !long)
continue

const keys = Object.keys(data).sort((a, b) => a.localeCompare(b))
const keys = Object.keys(data).sort((a, b) => a.localeCompare(b, 'en'))
if (!keys.length)
continue

Expand Down
2 changes: 1 addition & 1 deletion lib/help.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class Help extends BaseCommand {
if (aManNumber !== bManNumber)
return aManNumber - bManNumber

return a.localeCompare(b)
return a.localeCompare(b, 'en')
})
const man = mans[0]

Expand Down
2 changes: 1 addition & 1 deletion lib/ls.js
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ const augmentNodesWithMetadata = ({
}

const sortAlphabetically = (a, b) =>
a.pkgid.localeCompare(b.pkgid)
a.pkgid.localeCompare(b.pkgid, 'en')

const humanOutput = ({ color, result, seenItems, unicode }) => {
// we need to traverse the entire tree in order to determine which items
Expand Down
2 changes: 1 addition & 1 deletion lib/outdated.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class Outdated extends BaseCommand {
}))

// sorts list alphabetically
const outdated = this.list.sort((a, b) => a.name.localeCompare(b.name))
const outdated = this.list.sort((a, b) => a.name.localeCompare(b.name, 'en'))

// return if no outdated packages
if (outdated.length === 0 && !this.npm.config.get('json'))
Expand Down
2 changes: 1 addition & 1 deletion lib/utils/completion/installed-deep.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const installedDeep = async (npm) => {
})
.filter(i => (i.depth - 1) <= depth)
.sort((a, b) => a.depth - b.depth)
.sort((a, b) => a.depth === b.depth ? a.name.localeCompare(b.name) : 0)
.sort((a, b) => a.depth === b.depth ? a.name.localeCompare(b.name, 'en') : 0)

const res = new Set()
const gArb = new Arborist({ global: true, path: resolve(npm.globalDir, '..') })
Expand Down
2 changes: 1 addition & 1 deletion lib/utils/config/describe-all.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const describeAll = () => {
const sort = ([keya, {deprecated: depa}], [keyb, {deprecated: depb}]) => {
return depa && !depb ? 1
: !depa && depb ? -1
: keya.localeCompare(keyb)
: keya.localeCompare(keyb, 'en')
}
return Object.entries(definitions).sort(sort)
.map(([key, def]) => def.describe())
Expand Down
2 changes: 1 addition & 1 deletion lib/utils/npm-usage.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ const usages = (npm) => {
maxLen = Math.max(maxLen, c.length)
return set
}, [])
.sort((a, b) => a[0].localeCompare(b[0]))
.sort((a, b) => a[0].localeCompare(b[0], 'en'))
.map(([c, usage]) => `\n ${c}${' '.repeat(maxLen - c.length + 1)}${
(usage.split('\n').join('\n' + ' '.repeat(maxLen + 5)))}`)
.join('\n')
Expand Down
4 changes: 2 additions & 2 deletions lib/utils/tar.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,15 +76,15 @@ const getContents = async (manifest, tarball) => {
})

const comparator = (a, b) => {
return a.path.localeCompare(b.path, undefined, {
return a.path.localeCompare(b.path, 'en', {
sensitivity: 'case',
numeric: true,
})
}

const isUpper = (str) => {
const ch = str.charAt(0)
return ch >= 'A' && ch <= 'Z'
return ch === ch.toUpperCase()
}

const uppers = files.filter(file => isUpper(file.path))
Expand Down
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,9 @@
"Remove the 'files' below once we're done porting old tests over"
],
"tap": {
"test-env": [
"LC_ALL=sk"
],
"color": 1,
"files": "test/{lib,bin}",
"coverage-map": "test/coverage-map.js",
Expand Down
4 changes: 2 additions & 2 deletions scripts/bundle-and-gitignore-deps.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ arb.loadVirtual().then(tree => {
bundle.push(node.name)
}
}
pkg.bundleDependencies = bundle.sort((a, b) => a.localeCompare(b))
pkg.bundleDependencies = bundle.sort((a, b) => a.localeCompare(b, 'en'))

const ignores = shouldIgnore.sort((a, b) => a.localeCompare(b))
const ignores = shouldIgnore.sort((a, b) => a.localeCompare(b, 'en'))
.map(i => `/${i}`)
.join('\n')
const ignoreData = `# Automatically generated to ignore dev deps
Expand Down
4 changes: 2 additions & 2 deletions scripts/config-doc.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ const addShorthands = doc => {
const body = Object.entries(shorthands)
.sort(([shorta, expansiona], [shortb, expansionb]) => {
// sort by what they're short FOR
return expansiona.join(' ').localeCompare(expansionb.join(' ')) ||
shorta.localeCompare(shortb)
return expansiona.join(' ').localeCompare(expansionb.join(' '), 'en') ||
shorta.localeCompare(shortb, 'en')
})
.map(([short, expansion]) => {
const dash = short.length === 1 ? '-' : '--'
Expand Down
12 changes: 12 additions & 0 deletions tap-snapshots/test/lib/config.js.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ exports[`test/lib/config.js TAP config edit > should write config file 2`] = `
exports[`test/lib/config.js TAP config get no args > should list configs on config get no args 1`] = `
; "cli" config from command line options
cat = true
chai = true
dog = true
editor = "vi"
global = false
json = false
Expand All @@ -109,6 +112,9 @@ init.version = "1.0.0"
; "cli" config from command line options
cat = true
chai = true
dog = true
editor = "vi"
global = false
json = false
Expand All @@ -118,6 +124,9 @@ long = true
exports[`test/lib/config.js TAP config list > should list configs 1`] = `
; "cli" config from command line options
cat = true
chai = true
dog = true
editor = "vi"
global = false
json = false
Expand All @@ -132,6 +141,9 @@ long = false
exports[`test/lib/config.js TAP config list overrides > should list overridden configs 1`] = `
; "cli" config from command line options
cat = true
chai = true
dog = true
editor = "vi"
global = false
init.author.name = "Bar"
Expand Down
Loading

0 comments on commit 1a759f9

Please sign in to comment.