feat(opts): support full range of relevant CLI opts #19
Conversation
zkat
force-pushed
the
zkat/pacote-opts
branch
3 times, most recently
from
8bbbc85
to
e7b997b
Compare
| @@ -47,6 +46,9 @@ class Installer { | |||
| } | |||
| ) | |||
| }).then(() => { | |||
| return config(this.prefix, process.argv, this.pkg) | |||
There was a problem hiding this comment.
If we can avoid needing this.pkg for the UA, then this can be done as part of the previous join in parallel to reading package.json etc.
There was a problem hiding this comment.
Eventually this won't be slow so it won't matter anyway, and if it does matter, we can address later.
lib/config.js
Outdated
| if (_config) return BB.resolve(_config) | ||
| return readConfig().then(config => { | ||
| _config = { | ||
| config, |
There was a problem hiding this comment.
This needs to be passed to lifecycle for printing config as env variables.
lib/config.js
Outdated
| failOk: false, | ||
| force: config.force, | ||
| group: config.group, | ||
| ignorePrepublish: config['ignore-prepublish'], |
There was a problem hiding this comment.
The options in lifecycle are all camelcased. This will cause ignorePrepublish to not be respected.
lib/config.js
Outdated
| force: config.force, | ||
| group: config.group, | ||
| ignorePrepublish: config['ignore-prepublish'], | ||
| ignoreScripts: config['ignore-scripts'], |
lib/config.js
Outdated
| log, | ||
| production: config.production, | ||
| scriptShell: config['script-shell'], |
| production: config.production, | ||
| scriptShell: config['script-shell'], | ||
| scriptsPrependNodePath: config['scripts-prepend-node-path'], | ||
| unsafePerm: config['unsafe-perm'], |
| return opts | ||
| } | ||
|
|
||
| function calculateOwner () { |
There was a problem hiding this comment.
Seems like this logic can move into pacote itself.
There was a problem hiding this comment.
I'd rather not move this to pacote itself, because I'm trying to keep pacote easy to isolate: this involves keeping certain state around that only really makes sense to have at an application level. I'd rather not have pacote reading env vars.
lib/pacote-opts.js
Outdated
| strictSSL: npmOpts['strict-ssl'], | ||
| userAgent: npmOpts['user-agent'], | ||
|
|
||
| dmode: parseInt('0777', 8) & (~npmOpts['umask']), |
There was a problem hiding this comment.
Seems like this and fmode can move into pacote itself, considering its derived from umask
There was a problem hiding this comment.
I'm not sure pacote itself even needs to be setting these, since they are only used when a tarball doesn't contain mode details for file(s) it contains, which it should. This value is also the same as the default dmode in tar. Same with fmode below.
| _config = { | ||
| config, |
There was a problem hiding this comment.
This needs to be passed to lifecycle for printing config as env variables.
There was a problem hiding this comment.
Nvm, saw you preserved below after I commented.
lib/config.js
Outdated
| failOk: false, | ||
| force: config.force, | ||
| group: config.group, | ||
| ignorePrepublish: config['ignore-prepublish'], |
There was a problem hiding this comment.
We'll need a lifecycleOpts, to do the camelcasing!
lib/pacote-opts.js
Outdated
| strictSSL: npmOpts['strict-ssl'], | ||
| userAgent: npmOpts['user-agent'], | ||
|
|
||
| dmode: parseInt('0777', 8) & (~npmOpts['umask']), |
There was a problem hiding this comment.
I'm not sure pacote itself even needs to be setting these, since they are only used when a tarball doesn't contain mode details for file(s) it contains, which it should. This value is also the same as the default dmode in tar. Same with fmode below.
lib/pacote-opts.js
Outdated
|
|
||
| dmode: parseInt('0777', 8) & (~npmOpts['umask']), | ||
| fmode: parseInt('0666', 8) & (~npmOpts['umask']), | ||
| umask: npmOpts['umask'] |
There was a problem hiding this comment.
Is umask necessary? I can't really think of a scenario where cipm, npm, or even pacote would actually have a use for this. Especially since it is already set as part of the OS process and accessible via process.umask().
zkat
force-pushed
the
zkat/pacote-opts
branch
3 times, most recently
from
f80372b
to
eb63ae1
Compare
Fixes: #17