RFC for removing figgy-pudding from the CLI dep stack #102

isaacs · Feb 11, 2020

claudiahdz · Feb 11, 2020

Internally, we also use the following as our base-figgy-pudding config:

    baseConfig = NpmConfig(npm.config, {
      cache: path.join(npm.config.get('cache'), '_cacache'),
      color: !!npm.color,
      dirPacker: pack.packGitDep,
      hashAlgorithm: 'sha1',
      includeDeprecated: false,
      log,
      'npm-session': npmSession,
      'project-scope': npm.projectScope,
      refer: npm.referer,
      dmode: npm.modes.exec,
      fmode: npm.modes.file,
      umask: npm.modes.umask,
      npmVersion: npm.version,
      tmp: npm.tmp,
      Promise: BB
    })

We should clean that up too and make it clear which files/commands are actually in need of said config.

evocateur · Feb 12, 2020

accepted/0000-npm-option-handling.md

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


Until someone forgets to clone and mutates the config object passed down. I've seen that anti-pattern far too many times inside npm-owned code (with no equivalent to eslint no-param-reassign enabled) to be especially confident of this.

Especially when considering third-party consumers of said npm-owned libraries.

Also, leaving aside the parameter hygiene issue, you now have to document (or at least be aware of?) config that isn't relevant in the context of a given module. I'm less confident folks will apply this "just pass around a bag of options" pattern 100% consistently across 100% of the stack without some help, either via static analysis, really really good memories, or some other (hopefully) automated process.

So, figgy-pudding doesn't actually solve most of those problems. Config hygiene has still been a huge problem.

There is no One Weird Trick for this. It comes down to:

good docs that outline which configs are used

tests with 100% coverage

removing configs that are no longer relevant, and refactoring them when they don't make sense (see: only/also/production/optional/dev flags)

setting good defaults close to where they matter.

Third-party consumers of npm-owned libraries generally just passed in POJOs anyway, which would get figgy-ified. The problem is that once a module is using figgy-pudding, everything it uses must also use figgy-pudding, and it has to be careful not to accidentally exclude an option that one of its deps will end up using, so every level of the stack effectively has to know about every other. It's not encapsulation, it's the opposite of that. And no one can debug its peculiar restrictions.

We can explore doing something clever with config management later, but for now, nothing, and just having good tests, is far better than the something we have.

Until someone forgets to clone and mutates the config object passed down.

The recent move to 100% test coverage in all new code and for all dependencies has caught a ton of these types of problems, with far less hazardous side effects or bizarre restrictions imposed by figgy-pudding.

Is there a reason not to Object.freeze it, and easily force the cloning?

Is there a reason not to Object.freeze it, and easily force the cloning?

I don't see why not.

A frozen options object goes a long way toward mitigating my concerns (many of which, yes, no program can fix entirely).

BREAKING CHANGE: this removes figgy-pudding, and drops several option aliases. Defaults and behavior are all the same, and this module is now using the canonical camelCase option names that npm v7 will provide to all its deps. Related to: npm/rfcs#102

isaacs · Feb 14, 2020

On the documentation angle, the plan that we've adopted as a team is:

Update deps that are using non-canonical option names. (Only offenders turn out to be npm-registry-fetch, which uses css-case names instead of camelCase, and npm-pick-manifest, which uses the cute enjoyBy instead of the canonical before.)
Add the flat options object to the cli, start passing that instead of a fp object. (This actually works just fine for the fp-enabled deps. They just pass a plain object to fp instead passing an fp proxy, and it uses fp from there onwards.) This should fix the issues we're having trying to migrate to pacote 10, which is not fp enabled.
Bump major and convert npm-registry-fetch, ssri, npm-pick-manifest, cacache to use options object instead of fp.
Bump major, convert from fp to pojo opts, and update deps in all the libnpm* and npm-profile packages.
Pull all the updated deps into the CLI.

At every point, when updating a dep to use pojo options instead of fp:

Update code to set defaults in an intelligible way. (This varies, depending on how many defaults it sets, what it uses them for, etc.)
Document options that are relevant to this module.
If the options object is mutated, document this. (Exceedingly rare, but eg SSRI and pacote depend on this for updating integrity, size, etc.)
If the options object is mutated and not cloned, then definitely document this. (I haven't found any cases where this isn't a bug, so I'd be surprised if anything makes it through this refactor with this property intact, but leaving open the possibility that there's a use case we've failed to consider.)
Document which modules this package's options object is sent to. (Eg, Arborist sends its options object to pacote, pacote sends it to SSRI, cacache, and npm-registry-fetch, etc.) So, while it might not be feasible or maintenance-cost-effective to document every option that any dep might need and keep it up to date, at least we can send the reader in the right direction to compile that list for themselves when they need it.
100% test coverage. Always and everywhere. If you're ripping up the ground anyway, may as well put down something less grotty in its place.

BREAKING CHANGE: this removes figgy-pudding, and drops several option aliases. Defaults and behavior are all the same, and this module is now using the canonical camelCase option names that npm v7 will provide to all its deps. Related to: npm/rfcs#102 PR-URL: #22 Credit: @isaacs Close: #22 Reviewed-by: @claudiahdz

darcyclarke

✅ LGTM

darcyclarke · Feb 18, 2020

accepted/0000-npm-option-handling.md

+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 n options object with these canonically named


Looks like a typo slipped through (ie. ...create "an" options... unless you mean n in terms of Big O notation

"an" == "n" where n === 1. Which it does in this case. (I'll fix the typo, thanks :)

isaacs requested review from claudiahdz and mikemimik February 11, 2020 23:33

isaacs force-pushed the isaacs/internal-de-figgy-pudding branch from ae26205 to 3ba4fb8 Compare February 11, 2020 23:37

isaacs mentioned this pull request Feb 12, 2020

fix: remove figgy pudding npm/npm-pick-manifest#5

Merged

darcyclarke mentioned this pull request Feb 12, 2020

Remove Figgy Pudding from Deps npm/statusboard#66

Closed

evocateur reviewed Feb 12, 2020

View reviewed changes

isaacs mentioned this pull request Feb 14, 2020

wip: create a flat options object to pass to deps npm/cli#809

Closed

darcyclarke approved these changes Feb 18, 2020

View reviewed changes

RFC for removing figgy-pudding from the CLI dep stack

b44000b

isaacs force-pushed the isaacs/internal-de-figgy-pudding branch from 3ba4fb8 to b44000b Compare February 18, 2020 17:09

isaacs closed this in ddd9793 Mar 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

RFC for removing figgy-pudding from the CLI dep stack #102

RFC for removing figgy-pudding from the CLI dep stack #102

Uh oh!

isaacs commented Feb 11, 2020 •

edited

Loading

Uh oh!

claudiahdz commented Feb 11, 2020

Uh oh!

evocateur Feb 12, 2020

Uh oh!

evocateur Feb 12, 2020 •

edited

Loading

Uh oh!

isaacs Feb 14, 2020

Uh oh!

isaacs Feb 14, 2020

Uh oh!

ljharb Feb 14, 2020

Uh oh!

isaacs Feb 14, 2020

Uh oh!

evocateur Feb 14, 2020

Uh oh!

isaacs commented Feb 14, 2020 •

edited

Loading

Uh oh!

darcyclarke left a comment

Uh oh!

darcyclarke Feb 18, 2020

Uh oh!

isaacs Feb 18, 2020

Uh oh!

Uh oh!

Search code, repositories, users, issues, pull requests...

RFC for removing figgy-pudding from the CLI dep stack #102

RFC for removing figgy-pudding from the CLI dep stack #102

Uh oh!

Conversation

isaacs commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claudiahdz commented Feb 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evocateur Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isaacs commented Feb 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darcyclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

isaacs commented Feb 11, 2020 •

edited

Loading

evocateur Feb 12, 2020 •

edited

Loading

isaacs commented Feb 14, 2020 •

edited

Loading