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

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

Closed
wants to merge 1 commit into from

Conversation

nlf
Copy link
Contributor

@nlf nlf commented Jun 26, 2021

this may incite some further bike shedding, both for the name of the parameter and its allowed values. for now, i elected to take the simplest solution of having one location parameter instead of making it specific to the use (i.e. --config-location) and for the values i went with what @npmcli/config uses since it means i can just pass the config straight through.

it may warrant some further documentation about exactly what files each value refers to, but i wasn't sure where the appropriate place to put that was so this is a starting point to push the discussion along.

@nlf nlf requested a review from a team as a code owner June 26, 2021 16:01

* 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.

// 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.

@darcyclarke darcyclarke added Release 7.x work is associated with a specific npm 7 release semver:minor new backwards-compatible feature release: next These items should be addressed in the next release labels Jul 12, 2021
@wraithgar
Copy link
Member

This is an iterative change towards where we want to be, param name is fine it's been bikeshedded enough already. We can update the npm config list and npm config get outside of this PR

ruyadorno pushed a commit that referenced this pull request Jul 15, 2021
@ruyadorno
Copy link
Collaborator

Landed in 98905ae

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: next These items should be addressed in the next release Release 7.x work is associated with a specific npm 7 release semver:minor new backwards-compatible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants