diff --git a/accepted/0000-npm-option-handling.md b/accepted/0000-npm-option-handling.md new file mode 100644 index 000000000..468be4a9a --- /dev/null +++ b/accepted/0000-npm-option-handling.md @@ -0,0 +1,190 @@ +# Handling of npm config options in the CLI and dep stack + +NB: this is an internal RFC related to the code structure and patterns of +the CLI and its dependencies. No user-facing changes are contemplated by +this proposal. + +## Summary + +- Replace usage of `figgy-pudding` with plain JavaScript option objects. +- Document and define defaults for configs used by each module in the + stack. +- Clone options objects if they may be mutated in the future. +- Pass options objects intact to components further down the stack. +- Canonicalize option names to match consistent `camelCase` format. + +## Motivation + +The goal of FiggyPudding was to get the npm CLI and its associated +dependencies away from having a shared `npmconf` god-object with `.get()` +and `.set()` methods, which could be mutated at any time, and contain any +arbitrary fields. + +However, the use of FiggyPudding has led in practice to added complexity +and an even _more_ entrenched reliance on a god-object pattern, albeit one +that is even more opinionated and harder to use than `npmconf` was. + +In one example where this has proven particularly challenging, `npm ci` +does not get the full set of config values, and thus it does not properly +respect config values that would be handled if the user ran `npm install`. +Since _all_ package tree generation and reification will be handled by an +external dependency in npm v7 (ie, `@npmcli/arborist`), this will become an +even bigger issue. + +## Detailed Explanation + +In all of the CLI's dependencies, remove `figgy-pudding`, and replace with +an options object that uses the canonical names for npm config values. + +In the npm CLI, create an options object with these canonically named +fields, applying defaults and resolving aliases, as appropriate, using the +`npm.config` object as its source of truth. + +### Canonical Naming + +Use the `camelCase` forms of all config fields. For example, if the user +runs `npm install --prefer-online`, then the config options object would +contain `{ preferOnline: true }`. + +Thus, the npm CLI's dependencies should use `options.preferOnline` instead +of `options['prefer-online']`. + +## Rationale and Alternatives + +It's worth noting the problem that figgy-pudding aims to solve and the value +it currently provides, and ways that we will continue to solve that problem +and gain that value. + +1. A figgy-pudding config object is a self-documenting list of the options + that a given module uses, with defaults, canonical name, and so on. +2. Figgy-pudding allows a module to have a _different_ set of respected + configs than it provides to the modules deeper in the stack, because + they all proxy to the same underlying data object. +3. Figgy-pudding prevents any accidental mutation of the options provided + to a module. +4. Use of figgy-pudding avoids tight coupling to an `npmconf` object with + `get` and `set` methods. + +The response to these points is: + +1. It is better to document the list of options that a module uses _in its + documentation_, and make it clear in the code what their default values + are. For example, instead of: + + ```js + const MyOptions = figgyPudding({ + someFoo: { default: 'foo' }, + myBar: {}, + }) + + module.exports = (options) => { + options = MyOptions(options) + // ...options.someFoo + } + ``` + + We will now do: + + ```js + module.exports = (options = {}) => { + const { someFoo = 'foo' } = options + // doSomething(someFoo) + } + ``` + + and document the `someFoo` field in the package `README.md`. + +2. When a module in the CLI dependency stack needs to call out to one lower + down (for example, arborist calling pacote, which in turn calls + npm-registry-fetch, which then calls make-fetch-happen, etc.), we can + just pass the options object to the deeper dep. + + It's _fine_, really. + +3. When an options object might be mutated, it should be cloned first. For + example, instead of: + + ```js + class Foo { + constructor (options) { + this.options = options + // ... later ... + this.options.foo = 'foo' + } + } + ``` + + we will now do this: + + ```js + class Foo { + constructor (options = {}) { + this.options = { ...options } + } + } + ``` + +4. We achieve the same benefit by passing plain old JavaScript objects as + arguments rather than sharing an npmconf object. The `npm.config` + object will only be used to create the config object that gets passed to + dependencies, it will never be shared directly. + +Some alternatives: + +### Continue Using Figgy Pudding + +One alternative is to update tar, pacote v10, and arborist to use +figgy-pudding as well, and fix the issues that make it a challenge in other +scenarios. + +It is our assessment that doing so involves more work than simply removing +figgy pudding from the CLI entirely, and solving the problems in a +different way. + +### Create a less opinionated/easier to use/less read-restricted version + +We could update figgy-pudding to be less of a pain to work with. However, +we would remain in a mode of having a very unusual way of passing around +option arguments to functions and classes, which runs against the grain of +JavaScript development. And, reducing the restrictions of figgy-pudding +would tend to also reduce its benefits. + +Ultimately, we don't need something to _enforce_ config object and +documentation discipline, we just need to be disciplined about config +object usage and documentation. + +## Implementation + +Note: all of this is planned for npm v7. It has no effect on npm v6, or +any versions of any deps used by npm v6 and before. + +While making changes to deps and the CLI, update documentation to specify +which config values are relevant, and consider adding tests to ensure that +provided config objects cannot be inappropriately mutated. + +### Phase 1 - Canonicalize Config Names, provide POJO config + +- Create a list of all the config names and their canonical names. +- Update the CLI to create this object and pass it to dependencies instead of + the figgy-pudding object currently created in + `lib/config/figgy-config.js`. + +This will require updating some cases where css-case config keys are used +rather than camelCase, but otherwise, should be relatively straightforward. +Anything that takes a FiggyPudding object can also take a regular object as +an argument. + +### Phase 2 - Deepest Nested Deps + +- Create semver-major updates to `ssri`, `npm-registry-fetch`, + `npm-pick-manifest`, and `cacache` to take a plain old config object + instead of a figgy-pudding object, and use canonical config keys. +- Update direct usage of these within the CLI to provide the plain + JavaScript object instead of the FiggyConfig object. + +### Phase 3 - `libnpm*` Deps + +- Update `npm-profile` and all of the `libnpm*` dependencies (except for + `libcipm`, which will be removed in npm v7 and replaced by arborist) to + use the canonicalized pojo config object, and pass the same downstack to + their deps.