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

Isaacs/lifecycle envs #999

Merged
merged 10 commits into from
Mar 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ auditCmd.completion = function (opts, cb) {
}
}

class Auditor extends Installer {
class Auditor extends (class {}) {
constructor (where, dryrun, args, opts) {
super(where, dryrun, args, opts)
this.deepArgs = (opts && opts.deepArgs) || []
Expand Down
10 changes: 7 additions & 3 deletions lib/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -269,9 +269,13 @@ function list (cb) {
var defKeys = getKeys(defaults)
msg += '; default values\n'
defKeys.forEach(function (k) {
if (defaults[k] && typeof defaults[k] === 'object') return
var val = JSON.stringify(defaults[k])
if (defaults[k] !== npm.config.get(k)) {
const def = defaults[k]
// XXX remove this skip when we stop putting log in the npm.config set.
if (def && typeof def === 'object' && !Array.isArray(def)) {
return
}
var val = JSON.stringify(def)
if (def !== npm.config.get(k)) {
msg += '; ' + k + ' = ' + val + ' (overridden)\n'
} else msg += k + ' = ' + val + '\n'
})
Expand Down
26 changes: 13 additions & 13 deletions lib/config/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -356,25 +356,25 @@ Conf.prototype.addEnv = function (env) {
return CC.prototype.addEnv.call(this, '', conf, 'env')
}

function parseField (f, k) {
function parseField (f, k, listElement = false) {
if (typeof f !== 'string' && !(f instanceof String)) return f

// type can be an array or single thing.
var typeList = [].concat(types[k])
var isPath = typeList.indexOf(path) !== -1
var isBool = typeList.indexOf(Boolean) !== -1
var isString = typeList.indexOf(String) !== -1
var isUmask = typeList.indexOf(Umask) !== -1
var isNumber = typeList.indexOf(Number) !== -1
const typeList = [].concat(types[k])
const isPath = typeList.includes(path)
const isBool = typeList.includes(Boolean)
const isString = typeList.includes(String)
const isUmask = typeList.includes(Umask)
const isNumber = typeList.includes(Number)
const isList = typeList.includes(Array) && !listElement

f = ('' + f).trim()

if (f.match(/^".*"$/)) {
try {
f = JSON.parse(f)
} catch (e) {
throw new Error('Failed parsing JSON config key ' + k + ': ' + f)
}
// list types get put in the environment separated by double-\n
// usually a single \n would suffice, but ca/cert configs can contain
// line breaks and multiple entries.
if (isList) {
return f.split('\n').map(field => parseField(field, k, true))
}

if (isBool && !isString && f === '') return true
Expand Down
4 changes: 0 additions & 4 deletions lib/config/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ var path = require('path')
var url = require('url')
var Stream = require('stream').Stream
var semver = require('semver')
var stableFamily = semver.parse(process.version)
var nopt = require('nopt')
var os = require('os')
var osenv = require('osenv')
Expand Down Expand Up @@ -68,9 +67,6 @@ nopt.invalidHandler = function (k, val, type) {
}
}

if (!stableFamily || (+stableFamily.minor % 2)) stableFamily = null
else stableFamily = stableFamily.major + '.' + stableFamily.minor

var defaults

var temp = osenv.tmpdir()
Expand Down
8 changes: 6 additions & 2 deletions lib/config/flat-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ const flatOptions = npm => npm.flatOptions || Object.freeze({
color: !!npm.color,
includeStaged: npm.config.get('include-staged'),

preferDedupe: npm.config.get('prefer-dedupe'),
ignoreScripts: npm.config.get('ignore-scripts'),

projectScope: npm.projectScope,
npmVersion: npm.version,
nodeVersion: npm.config.get('node-version'),
Expand All @@ -57,7 +60,8 @@ const flatOptions = npm => npm.flatOptions || Object.freeze({
localPrefix: npm.localPrefix,
global: npm.config.get('global'),

metricsRegistry: npm.config.get('metrics-registry'),
metricsRegistry: npm.config.get('metrics-registry') ||
npm.config.get('registry'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Are these the same thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless specified explicitly, metrics-registry defaults to the public registry.

sendMetrics: npm.config.get('send-metrics'),
registry: npm.config.get('registry'),
get scope () {
Expand Down Expand Up @@ -159,7 +163,7 @@ const flatOptions = npm => npm.flatOptions || Object.freeze({
packageLockOnly: npm.config.get('package-lock-only'),
globalStyle: npm.config.get('global-style'),
legacyBundling: npm.config.get('legacy-bundling'),
scriptShell: npm.config.get('script-shell'),
scriptShell: npm.config.get('script-shell') || undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: What's the default returned value of npm.config.get('...') which doesn't find the key ...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can be literally anything.

$ npm config get script-shell --script-shell=420lol
420lol

But @npmcli/run-script only allows Boolean or undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Er, only allows strings. (Sorry, was thinkign of another flag.)

So this is the case that would be a problem:

npm run foo --no-script-shell

And the effect of setting that false value to undefined is to fall back to the system default (/bin/sh or cmd.exe).

omit: buildOmitList(npm),

// used to build up the appropriate {add:{...}} options to Arborist.reify
Expand Down
61 changes: 61 additions & 0 deletions lib/config/set-envs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// Set environment variables for any non-default configs,
// so that they're already there when we run lifecycle scripts.
//
// See https://github.com/npm/rfcs/pull/90

const envName = name => `npm_config_${name.replace(/-/g, '_')}`
mikemimik marked this conversation as resolved.
Show resolved Hide resolved

// Return the env key if this is a thing that belongs in the env.
// Ie, if the key isn't a @scope, //nerf.dart, or _private,
// and the value is a string or array. Otherwise return false.
const envKey = (key, val) => {
return !/^[\/@_]/.test(key) &&
mikemimik marked this conversation as resolved.
Show resolved Hide resolved
(typeof val === 'string' || Array.isArray(val)) &&
`npm_config_${key.replace(/-/g, '_').toLowerCase()}`
mikemimik marked this conversation as resolved.
Show resolved Hide resolved
}

const envVal = val => Array.isArray(val) ? val.join('\n\n') : val

const sameConfigValue = (def, val) =>
!Array.isArray(val) || !Array.isArray(def) ? def === val
: sameArrayValue(def, val)

const sameArrayValue = (def, val) => {
if (def.length !== val.length) {
return false
}
for (let i = 0; i < def.length; i++) {
if (def[i] !== val[i]) {
return false
}
}
return true
}

const setEnvs = npm => {
// The objects in the config.list array are arranged in
// a prototype chain, so we can just for/in over the top
// of the stack and grab any that don't match the default
const { config: { list: [ configs ] } } = npm
const { defaults } = require('./defaults.js')
const set = {}
for (const key in configs) {
const val = configs[key]
const environ = envKey(key, val)
if (!sameConfigValue(defaults[key], val) && environ) {
process.env[environ] = envVal(val)
}
}

process.env.npm_execpath = require.main.filename
process.env.npm_node_execpath = process.execPath
process.env.npm_command = npm.command

// note: this doesn't afect the *current* node process, of course, since
// it's already started, but it does affect the options passed to scripts.
if (configs['node-options']) {
process.env.NODE_OPTIONS = configs['node-options']
}
}

module.exports = setEnvs
4 changes: 3 additions & 1 deletion lib/dedupe.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// XXX replace this with @npmcli/arborist

var util = require('util')
var path = require('path')
var validate = require('aproba')
Expand Down Expand Up @@ -47,7 +49,7 @@ function Deduper (where, dryrun) {
this.noPackageJsonOk = true
this.topLevelLifecycles = false
}
util.inherits(Deduper, Installer)
util.inherits(Deduper, class {}) // Installer)

Deduper.prototype.loadIdealTree = function (cb) {
validate('F', arguments)
Expand Down
4 changes: 4 additions & 0 deletions lib/help.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ var shorthands = require('./config/cmd-list').shorthands
var commands = cmdList.concat(Object.keys(shorthands))
var output = require('./utils/output.js')

const usage = require('./utils/usage.js')

help.usage = usage('help', 'npm help <term> [<terms..>]')

function help (args, cb) {
var argv = npm.config.get('argv').cooked

Expand Down
2 changes: 2 additions & 0 deletions lib/hook.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ module.exports = (args, cb) => Promise.resolve().then(() => {
.catch(err => err.code === 'EUSAGE' ? cb(err.message) : cb(err))
})

module.exports.usage = hook.usage

function hook (args) {
return otplease(npm.flatOptions, opts => {
switch (args[0]) {
Expand Down
43 changes: 10 additions & 33 deletions lib/npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,19 +92,11 @@
var commandCache = {}
var aliasNames = Object.keys(aliases)

var littleGuys = [ 'isntall', 'verison' ]
var fullList = cmdList.concat(aliasNames).filter(function (c) {
var fullList = npm.fullList = cmdList.concat(aliasNames).filter(c => {
return plumbing.indexOf(c) === -1
})
var abbrevs = abbrev(fullList)

// we have our reasons
fullList = npm.fullList = fullList.filter(function (c) {
return littleGuys.indexOf(c) === -1
})

var registryRefer

Object.keys(abbrevs).concat(plumbing).forEach(function addCommand (c) {
Object.defineProperty(npm.commands, c, { get: function () {
if (!loaded) {
Expand All @@ -113,19 +105,20 @@
'See the README.md or bin/npm-cli.js for example usage.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Line 106: var registryRefer is defined by never used
Line 95: var littleGuys is a reference to incorrect spelling, but lib/config/cmd-list.js already has an affordances section; shouldn't we be using that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, registryRefer should be removed, since we're not sending the referer header any more.

Istr there's a difference between "affordances" and "littleGuys", but I don't recall it offhand. Will dig in and sort that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, no difference. Just apparently something that got overlooked 4 years ago when it stopped being relevant.

)
}
var a = npm.deref(c)
var actualCommand = npm.deref(c)
if (c === 'la' || c === 'll') {
npm.config.set('long', true)
}

npm.command = c
npm.config.set('command', c)
npm.command = actualCommand
npm.flatOptions = require('./config/flat-options.js')(npm)
if (commandCache[a]) return commandCache[a]
require('./config/set-envs.js')(npm)

var cmd = require(path.join(__dirname, a + '.js'))
if (commandCache[actualCommand]) return commandCache[actualCommand]

commandCache[a] = function () {
var cmd = require(path.join(__dirname, actualCommand + '.js'))

commandCache[actualCommand] = function () {
var args = Array.prototype.slice.call(arguments, 0)
if (typeof args[args.length - 1] !== 'function') {
args.push(defaultCb)
Expand All @@ -140,30 +133,14 @@
}
})

if (!registryRefer) {
registryRefer = [a].concat(args[0]).map(function (arg) {
// exclude anything that might be a URL, path, or private module
// Those things will always have a slash in them somewhere
if (arg && arg.match && arg.match(/\/|\\/)) {
return '[REDACTED]'
} else {
return arg
}
}).filter(function (arg) {
return arg && arg.match
}).join(' ')
npm.referer = registryRefer
npm.config.set('refer', npm.referer)
}

cmd.apply(npm, args)
}

Object.keys(cmd).forEach(function (k) {
commandCache[a][k] = cmd[k]
commandCache[actualCommand][k] = cmd[k]
})

return commandCache[a]
return commandCache[actualCommand]
},
enumerable: fullList.indexOf(c) !== -1,
configurable: true })
Expand Down
3 changes: 2 additions & 1 deletion lib/prune.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// XXX replace this with @npmcli/arborist
// prune extraneous packages.

module.exports = prune
Expand Down Expand Up @@ -28,7 +29,7 @@ function Pruner (where, dryrun, args) {
Installer.call(this, where, dryrun, args)
this.autoPrune = true
}
util.inherits(Pruner, Installer)
util.inherits(Pruner, class {}) // Installer)

Pruner.prototype.loadAllDepsIntoIdealTree = function (cb) {
log.silly('uninstall', 'loadAllDepsIntoIdealTree')
Expand Down
Loading