-
Notifications
You must be signed in to change notification settings - Fork 4k
wip: create a flat options object to pass to deps #809
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
Conversation
|
|
||
| const flatOptions = npm => ({ | ||
| // Note that many of these do not come from configs or cli flags | ||
| // per se, though they may be implied or defined by them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking as someone who maintains a CLI (lerna) that implements npm publishing using current libnpm* packages (with an alarming amount of copypasta from npm's source) and attempts to stay as consistent as possible with npm CLI patterns, extracting "How npm reads and serializes its configuration" into a separate package would be a great boon.
I realize it's extremely low on the priority list, but I just wanted to register the sentiment while it's top-of-mind, and before I inevitably feel the urge to make more copypasta...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that would be really nice, I agree. (Also speaking as someone who maintains and uses a lot of npm deps ;)
We had once upon a time a package called npmconf that housed all of the config reading and management as well as all the default values. The problem was that every time we wanted to add or modify a config value, it meant an update in a bunch of disparate places, and it was kind of a pain to manage, so we folded it back in. This gets more convoluted when we have a bunch of other modules expecting to get a preferOnline option and need to know to convert that from config.get('prefer-online') (or even config.get('cache-min')<=0). The goal of figgy-pudding was to make this easier, but in practice, has made it much harder, which is why we're moving to the simplest possible approach, and from there can build up something a bit more maintainable. Just taking an inventory of all our configs as part of this upgrade process has been really enlightening. The clever technical solution didn't actually get at the root of the problem, and doing the work to dredge it all up and clean the actual mess has been sorely needed.
Some day, I'd really like to consider dropping nopt in favor of something a bit more simple and declarative, like jackspeak or even yargs, and then have a package that doesn't handle defaults, but just reads and updates config files, so that the set of defaults and expected types can still live in npm/cli maybe. It's a weird sort of animal to cut in half, though, because I do want to be able to provide some feedback to the user that --umask=notanumber is an invalid config value. So I'm not sure exactly what that'll look like. We may be in this "create a pojo and share it" mode for a while, most likely at least through the v7 release line. It just makes it so much easier to manage our deps if they can get an options object like any other JavaScript module out there does, without the extra translation layer of fp.
|
Suggestion was made (and I think it's a good ideal) to |
27e96cd to
2265302
Compare
1421928 to
ed30363
Compare
|
The good bits of this landed already in the feature/update-deps-v1 branch, so I'm closing this PR. |
Related to npm/rfcs#102.
wip, needs tests.