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

Jump to bottom

feat: introduce 'location' parameter for npm config command #3471

Closed
wants to merge 1 commit into from
Closed

feat: introduce 'location' parameter for npm config command #3471

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/content/commands/npm-config.md
View file
Open in desktop
Expand Up @@ -128,6 +128,14 @@ folder instead of the current working directory. See

The command to run for `npm edit` and `npm config edit`.

#### `location`

* Default: "user" unless `--global` is passed, which will also set this value
to "global"
* Type: "global", "user", or "project"

When passed to `npm config` this refers to which config file to use.

#### `long`

* Default: false
Expand Down
9 changes: 9 additions & 0 deletions docs/content/using-npm/config.md
View file
Open in desktop
Expand Up @@ -67,6 +67,7 @@ The following shorthands are parsed on the command-line:
* `--desc`: `--description`
* `-f`: `--force`
* `-g`: `--global`
* `-L`: `--location`
* `-d`: `--loglevel info`
* `-s`: `--loglevel silent`
* `--silent`: `--loglevel silent`
Expand Down Expand Up @@ -753,6 +754,14 @@ Used with `npm ls`, limiting output to only those packages that are linked.
The IP address of the local interface to use when making connections to the
npm registry. Must be IPv4 in versions of Node prior to 0.12.

#### `location`

* Default: "user" unless `--global` is passed, which will also set this value
to "global"
* Type: "global", "user", or "project"
Copy link
Collaborator

Choose a reason for hiding this comment

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

built-in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

leaving that out was intentional, feels like the built-in use case should be a file overwritten by packagers and not something a user would interactively change on their own to me. I'm definitely open to discuss it though

Copy link
Collaborator

Choose a reason for hiding this comment

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

I’d expect it to be undocumented regardless, but it seems pretty useful for debugging the source of an unexpected default config (for “get”), and it seems weird to allow it for get and not for set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get and list both ignore the parameter entirely. they aren't location specific, they return whatever the most prioritized value(s) is/are. we could allow for the location flag on them as a means of saying "show me only data from this location" in which case I agree exposing built-in would be appropriate. if we did that, though, we would have to remove the default value for the location and have separate behaviors in the commands for a value of null which i don't love...

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah hm, that's a fair point.

if it's only going to work in set then omitting built-in makes sense; but it seems more useful to me if it works for all of get/list/set, and includes built-in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to address the issue of debugging the source of an unexpected config value, how about we allow npm config list to accept config keys for which it will list everywhere that key is defined and what overwrote what.

so npm config get key will continue to return a single value representing the value of key from its highest prioritized source, and npm config list -l key will list every value that key has in any source in a way that makes it clear why it was overridden from other sources.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For list, that sounds great.


When passed to `npm config` this refers to which config file to use.

#### `loglevel`

* Default: "notice"
Expand Down
8 changes: 4 additions & 4 deletions lib/config.js
View file
Open in desktop
Expand Up @@ -56,6 +56,7 @@ class Config extends BaseCommand {
'json',
'global',
'editor',
'location',
'long',
]
}
Expand Down Expand Up @@ -137,7 +138,7 @@ class Config extends BaseCommand {
if (!args.length)
throw this.usageError()

const where = this.npm.config.get('global') ? 'global' : 'user'
const where = this.npm.config.get('location')
for (const [key, val] of Object.entries(keyValues(args))) {
this.npm.log.info('config', 'set %j %j', key, val)
this.npm.config.set(key, val || '', where)
Expand Down Expand Up @@ -167,16 +168,15 @@ class Config extends BaseCommand {
if (!keys.length)
throw this.usageError()

const where = this.npm.config.get('global') ? 'global' : 'user'
const where = this.npm.config.get('location')
for (const key of keys)
this.npm.config.delete(key, where)
await this.npm.config.save(where)
}

async edit () {
const global = this.npm.config.get('global')
const e = this.npm.config.get('editor')
const where = global ? 'global' : 'user'
const where = this.npm.config.get('location')
const file = this.npm.config.data.get(where).source

// save first, just to make sure it's synced up
Expand Down
25 changes: 25 additions & 0 deletions lib/utils/config/definitions.js
View file
Open in desktop
Expand Up @@ -1103,6 +1103,31 @@ define('local-address', {
flatten,
})

define('location', {
default: 'user',
short: 'L',
type: [
'global',
'user',
'project',
],
defaultDescription: `
"user" unless \`--global\` is passed, which will also set this value to "global"
`,
description: `
When passed to \`npm config\` this refers to which config file to use.
`,
// NOTE: the flattener here deliberately does not alter the value of global
// for now, this is to avoid inadvertently causing any breakage. the value of
// global, however, does modify this flag.
flatten (key, obj, flatOptions) {
// if global is set, we override ourselves
Copy link
Member

Choose a reason for hiding this comment

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

Should this log a warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is here just to maintain the previous behavior of npm config respecting --global, and since at this point that's all this flag even goes to i think it's fine to not warn on it.

if (obj.global)
obj.location = 'global'
Comment on lines +1125 to +1126
Copy link

Choose a reason for hiding this comment

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

This logic doesn't appear to be working correctly. See #3572.

flatOptions.location = obj.location
},
})

define('loglevel', {
default: 'notice',
type: [
Expand Down
10 changes: 5 additions & 5 deletions tap-snapshots/test/lib/config.js.test.cjs
View file
Open in desktop
Expand Up @@ -5,7 +5,7 @@
* Make sure to inspect the output below. Do not ignore changes!
*/
'use strict'
exports[`test/lib/config.js TAP config edit --global > should write global config file 1`] = `
exports[`test/lib/config.js TAP config edit --location=global > should write global config file 1`] = `
;;;;
; npm globalconfig file: /etc/npmrc
; this is a simple ini-formatted file
Expand Down Expand Up @@ -92,8 +92,8 @@ cat = true
chai = true
dog = true
editor = "vi"
global = false
json = false
location = "user"
long = false

; node bin location = /path/to/node
Expand All @@ -116,8 +116,8 @@ cat = true
chai = true
dog = true
editor = "vi"
global = false
json = false
location = "user"
long = true
`

Expand All @@ -128,8 +128,8 @@ cat = true
chai = true
dog = true
editor = "vi"
global = false
json = false
location = "user"
long = false

; node bin location = /path/to/node
Expand All @@ -145,9 +145,9 @@ cat = true
chai = true
dog = true
editor = "vi"
global = false
init.author.name = "Bar"
json = false
location = "user"
long = false

; "user" config from ~/.npmrc
Expand Down
3 changes: 2 additions & 1 deletion tap-snapshots/test/lib/load-all-commands.js.test.cjs
View file
Open in desktop
Expand Up @@ -151,7 +151,8 @@ npm config list [--json]
npm config edit

Options:
[--json] [-g|--global] [--editor <editor>] [-l|--long]
[--json] [-g|--global] [--editor <editor>] [-L|--location <global|user|project>]
[-l|--long]

alias: c

Expand Down
11 changes: 11 additions & 0 deletions tap-snapshots/test/lib/utils/config/definitions.js.test.cjs
View file
Open in desktop
Expand Up @@ -81,6 +81,7 @@ Array [
"legacy-peer-deps",
"link",
"local-address",
"location",
"loglevel",
"logs-max",
"long",
Expand Down Expand Up @@ -1019,6 +1020,16 @@ The IP address of the local interface to use when making connections to the
npm registry. Must be IPv4 in versions of Node prior to 0.12.
`

exports[`test/lib/utils/config/definitions.js TAP > config description for location 1`] = `
#### \`location\`

* Default: "user" unless \`--global\` is passed, which will also set this value
to "global"
* Type: "global", "user", or "project"

When passed to \`npm config\` this refers to which config file to use.
`

exports[`test/lib/utils/config/definitions.js TAP > config description for loglevel 1`] = `
#### \`loglevel\`

Expand Down
8 changes: 8 additions & 0 deletions tap-snapshots/test/lib/utils/config/describe-all.js.test.cjs
View file
Open in desktop
Expand Up @@ -632,6 +632,14 @@ Used with \`npm ls\`, limiting output to only those packages that are linked.
The IP address of the local interface to use when making connections to the
npm registry. Must be IPv4 in versions of Node prior to 0.12.

#### \`location\`

* Default: "user" unless \`--global\` is passed, which will also set this value
to "global"
* Type: "global", "user", or "project"

When passed to \`npm config\` this refers to which config file to use.

#### \`loglevel\`

* Default: "notice"
Expand Down
3 changes: 3 additions & 0 deletions tap-snapshots/test/lib/utils/config/index.js.test.cjs
View file
Open in desktop
Expand Up @@ -64,6 +64,9 @@ Object {
"l": Array [
"--long",
],
"L": Array [
"--location",
],
"local": Array [
"--no-global",
],
Expand Down
3 changes: 2 additions & 1 deletion tap-snapshots/test/lib/utils/npm-usage.js.test.cjs
View file
Open in desktop
Expand Up @@ -294,7 +294,8 @@ All commands:
npm config edit

Options:
[--json] [-g|--global] [--editor <editor>] [-l|--long]
[--json] [-g|--global] [--editor <editor>] [-L|--location <global|user|project>]
[-l|--long]

alias: c

Expand Down
26 changes: 13 additions & 13 deletions test/lib/config.js
View file
Open in desktop
Expand Up @@ -47,8 +47,8 @@ const defaults = {
const cliConfig = {
editor: 'vi',
json: false,
location: 'user',
long: false,
global: false,
cat: true,
chai: true,
dog: true,
Expand Down Expand Up @@ -198,8 +198,8 @@ t.test('config list --json', t => {
{
editor: 'vi',
json: true,
location: 'user',
long: false,
global: false,
cat: true,
chai: true,
dog: true,
Expand Down Expand Up @@ -265,7 +265,7 @@ t.test('config delete multiple key', t => {
})
})

t.test('config delete key --global', t => {
t.test('config delete key --location=global', t => {
t.plan(4)

npm.config.delete = (key, where) => {
Expand All @@ -277,13 +277,13 @@ t.test('config delete key --global', t => {
t.equal(where, 'global', 'should save global config post-delete')
}

cliConfig.global = true
cliConfig.location = 'global'
config.exec(['delete', 'foo'], (err) => {
t.error(err, 'npm config delete key --global')
t.error(err, 'npm config delete key --location=global')
})

t.teardown(() => {
cliConfig.global = false
cliConfig.location = 'user'
delete npm.config.delete
delete npm.config.save
})
Expand Down Expand Up @@ -419,7 +419,7 @@ t.test('config set invalid key', t => {
})
})

t.test('config set key --global', t => {
t.test('config set key --location=global', t => {
t.plan(5)

npm.config.set = (key, val, where) => {
Expand All @@ -432,13 +432,13 @@ t.test('config set key --global', t => {
t.equal(where, 'global', 'should save global config')
}

cliConfig.global = true
cliConfig.location = 'global'
config.exec(['set', 'foo', 'bar'], (err) => {
t.error(err, 'npm config set key --global')
t.error(err, 'npm config set key --location=global')
})

t.teardown(() => {
cliConfig.global = false
cliConfig.location = 'user'
delete npm.config.set
delete npm.config.save
})
Expand Down Expand Up @@ -583,10 +583,10 @@ sign-git-commit=true`
})
})

t.test('config edit --global', t => {
t.test('config edit --location=global', t => {
t.plan(6)

cliConfig.global = true
cliConfig.location = 'global'
const npmrc = 'init.author.name=Foo'
npm.config.data.set('global', {
source: '/etc/npmrc',
Expand Down Expand Up @@ -626,7 +626,7 @@ t.test('config edit --global', t => {
})

t.teardown(() => {
cliConfig.global = false
cliConfig.location = 'user'
npm.config.data.delete('user')
delete npm.config.save
})
Expand Down
23 changes: 23 additions & 0 deletions test/lib/utils/config/definitions.js
View file
Open in desktop
Expand Up @@ -797,3 +797,26 @@ t.test('save-exact', t => {
t.strictSame(flat, { savePrefix: '~1.2.3' })
t.end()
})

t.test('location', t => {
const obj = {
global: true,
location: 'user',
}
const flat = {}
definitions.location.flatten('location', obj, flat)
// global = true sets location in both places to global
t.strictSame(flat, { location: 'global' })
t.strictSame(obj, { global: true, location: 'global' })

obj.global = false
obj.location = 'user'
delete flat.global
delete flat.location

definitions.location.flatten('location', obj, flat)
// global = false leaves location unaltered
t.strictSame(flat, { location: 'user' })
t.strictSame(obj, { global: false, location: 'user' })
t.end()
})