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

RFC: Command Specific Configuration #566

Closed
wants to merge 1 commit into from

Conversation

darcyclarke
Copy link
Contributor

@darcyclarke darcyclarke changed the title feat: command specific configuration RFC: Command Specific Configuration Apr 6, 2022
@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label Apr 6, 2022
Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

yes please, this should have been in v1 :-D

@lukekarrys
Copy link
Contributor

Can there be an optional parent scope for a "config version"? It wouldn't be tied directly to the npm version but it would allow us to making breaking config changes and still honor older versions.

Concerns:

  • I'm not sure how to make this scope unambiguous when comparing with a command scope.
  • I'm not sure if there is a way to reset the scope in ini files
[config-version=3]

[npm-audit]
fix=true

[config-version=4]

[npm-audit]
fix=false

@ljharb
Copy link
Contributor

ljharb commented Apr 13, 2022

I don't think it'd be possible to have more than one config version at a time? since config can come from env vars, CLI args, and any of 4 possible npmrc files (5 if you include child workspaces), i think it's got to be an all or nothing thing.

@dominykas
Copy link

dominykas commented Apr 15, 2022

😍

Does that mean we'd be able to do

[npm-install]
ignore-scripts=true

?

How would that interact with other related commands, e.g. npm ci?

Might be out of scope here, but I guess npm update is somewhat different, but it does run install scripts or does it not?

@darcyclarke
Copy link
Contributor Author

@dominykas yes, your example is correct.

How would that interact with other related commands, e.g. npm ci?

This is a good question. IMO, your example shouldn't effect ci's behavior because it's an independent command (ref: https://github.com/npm/cli/tree/latest/lib/commands) but I could be swayed there maybe. There's probably other examples/caveats that should be considered & discussed (ex. npm install will run npm pack for git dependencies, if you have [npm-pack] specific config set, potentially that should be respected). Would love to discuss more in a call or async.

Might be out of scope here, but I guess npm update is somewhat different, but it does run install scripts or does it not?

You're right, there may be a case where you want to do something for install & update; but I feel like you can just duplicate that config if that's what you want. I guess the question to ask is whether we have commands that invoke other commands (ex. it & cit) that we should explicitly note either do or don't respect the command specific config for the subsequent commands they spawn.

@ljharb
Copy link
Contributor

ljharb commented Apr 18, 2022

It seems like they probably should - npm publish invokes npm pack, and i would expect any pack-specific config to be respected during a publish.

@dominykas
Copy link

dominykas commented Apr 19, 2022

I feel like you can just duplicate that config if that's what you want

Yeah, I agree - for both cases - ci and update and also for whatever executes other commands. The only concern here is documenting that and making sure that people who advocate a solution advocate the full solution... always hard :D And maybe there are ways to let the users know that e.g. they're running npm ci and some settings that apply to npm install will not be applied?

Edit: or maybe there's a way to find a syntax that applies to multiple commands?

@darcyclarke
Copy link
Contributor Author

Actions

  • needs command & config audit (ie. documenting which commands call other commands & what config should or shouldn't be shared with them: ex. npm install calls npm pack for git deps)

@coolaj86
Copy link

coolaj86 commented Aug 8, 2022

Why have a special ini-style syntax for a node program?

JSON.

Human readable.

Machine readable.

Easy to integrate with CI.

Easy to batch edit with yq (or a variety of other tools).

No special dependencies or parsers required.

No complexity from YAML.

No confusion with TOML.

You could easily auto-update existing .npmrc to .npmrc.json and never regret it.

I just happened across this due to npm/cli#5279, but it seems an oversight that .npmrc was ever not JSON (especially considering that comments aren't preserved anyway) and no better time to fix it than before it's too late. :)

@ljharb
Copy link
Contributor

ljharb commented Aug 8, 2022

@coolaj86 npmrc has been ini since time immemorial and that's unlikely to change; it doesn't really matter why it's that way, but it simply is. It is about 12 years "too late" to make such a breaking change.

@coolaj86
Copy link

coolaj86 commented Aug 8, 2022

It is about 12 years "too late"

Nonsense. The whole --init-author-name vs init-author-name vs init.author.name confusion is proving that it's never too late to make a breaking change.

And the whole npm set vs npm config set.

And a dozen of other instances since at least v5 onwards.

npm makes breaking changes all the time.

Making a breaking change that requires less custom code and that's easier to implement, verify, and automate will of course get the same backlash of any of the other breaking changes (XKCD: Workflow), but think of all the CI systems that will get to drop a few dozen dependencies and all the supply chain attacks that will save us from, and all the automation it will make easier.

init-author-name isn't helping with any of that and it got in just fine.

(hopefully most of the automation tooling updated to npm config set a long time ago, but if not, it's a good incentive, and a simplification to the existing process either way)

@nlf
Copy link
Contributor

nlf commented Oct 6, 2022

think of all the CI systems that will get to drop a few dozen dependencies and all the supply chain attacks that will save us from

just for perspective if we switched to JSON this would only drop one dependency, that being the ini package itself. it has no external dependencies, so the impact on the overall dep tree is extremely minimal.

@rondales
Copy link

Rendered RFC

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.

7 participants