RFC: reduce lifecycle script environment size

[isaacs]

see rfc

[isaacs]

(pulled reply to this comment out so it doesn't get lost when I fix the typo)

In particular, i use some of these fields in https://npmjs.com/safe-publish-latest in "prepublish"/"prepare"/"prepublishOnly" (just looked; seems i particularly use npm_config_tag), and also in "version"/"preversion"/"postversion" in a few packages. Hopefully the vars that are relevant to the particular script can be preserved?

Another approach might be, instead of only putting explicit cli configs in the environment, copy any configs that are not the default values? That would still be a larger environment, though less absurd than what we currently do. It would mean that you'll have to know latest is the default tag value.

Having to manage "which configs are relevant for the current command" is tricky. prepare is also run for git installs, for example, and most configs are "relevant" for installation.

[ljharb]

I’d be fine with providing me “not the defaults”; i think it’s totally reasonable to have to encode those, and it’s even easier if npm itself has an easy way to access them (like, if the env var is missing, i can call some shell command to print out the value, which i think i can already do with config values, just not super cheaply)

[isaacs]

if the env var is missing, i can call some shell command to print out the value, which i think i can already do with config values, just not super cheaply

Yep, you can do this now. Either just exec npm config get tag, or if you wanna be super careful to get exactly the same npm running the script, you can get it this way:

// fallbacks for absurdly old npm versions where these two environs were not provided
const node = process.env.npm_node_execpath || process.execPath
const npm = process.env.npm_execpath || 'npm'
const {spawnSync} = require('child_process')
const tag = spawnSync(node, [npm, 'config', 'get', 'tag']).stdout.toString().trim()

[ljharb]

The problem is that that command is incredibly slow (it’s the sole reason nvm has any performance problems, since it invokes npm config get prefix every use) - it’d be ideal to have a cheaper and easier way to do it (alternatively, if npm config get somehow got way faster)

[isaacs]

The slowest thing in npm config get's startup is reading the config files, which any such tool would have to do anyway, and the child process overhead, which again any tool would have to do.

The best approach for your use case is probably just const tag = process.env.npm_config_tag || 'latest'.

[ljharb]

Great, that's what I already have :-) and i'm totally fine with that as long as a non-default tag remains available in preversion, particularly. Thanks!

[ljharb]

It looks like the in-publish module also uses npm_config_argv: https://npmcdn.com/[email protected]/index.js, it'd be great not to break that one too :-)

[isaacs]

Hmm, npm_config_argv probably has to go. The way that in-publish is using it is particularly tied to the specifics of nopt, which means we could never migrate off of it onto a simpler or more well-supported argument parser. Also, with the advent of the prepublishOnly lifecycle hook, it seems mostly unnecessary. If you want a prepublish script that only runs for actual publish, use that. Since in-publish has not been updated in 5 years, this feels outside the bounds of reasonable support limits.

It would be better, if programs need to know the command being run, to have npm export a npm_command environment variable instead. We could send a PR to in-publish to check this value as well as looking at argv.

[ljharb]

I use in-publish heavily; that it's not been updated in so long is solely due to npm not having broken it, not because it shouldn't be supported imo.

It'd be fine if a patch of in-publish could be published that used an alternative method for npm 7+, of course! npm_config_argv doesn't have to stay if there's another way for it to detect that it's in a publish (or an install).

[isaacs]

It'd be fine if a patch of in-publish could be published that used an alternative method for npm 7+, of course! npm_config_argv doesn't have to stay if there's another way for it to detect that it's in a publish (or an install).

Yeah, I'll send a PR to @iarna to use env.npm_command if it's present. Let's just have the cli set that, and then you also won't have to sniff for ^i(n(s(t(a(l(l)?)?)?)?)?)? (which incidentally won't catch npm isntall!)

[vweevers]

More usage of npm_config_*:

• https://github.com/prebuild/prebuild-install/blob/84e610b433b2f3ee486761e27139fa3f368135b4/rc.js#L11-L27
• https://github.com/prebuild/prebuild-install/blob/84e610b433b2f3ee486761e27139fa3f368135b4/rc.js#L32-L49
• https://github.com/prebuild/node-gyp-build/blob/abd0b6411cc7c2c82c8493030d031b14e6cb9a3b/bin.js#L59-L75

[isaacs]

@vweevers So, I think "put non-default configs in the env" would satisfy most of prebuild-install's needs, but of course it'll require a code change for the argv bit. Essentially you'd have to just look at env.npm_config_* for each of the relevant options, rather than parse them out of the npm_config_argv set. And, that's probably actually a better approach today anyway, since I imagine you'd want npm_config_download=foo npm install to be treated the same as npm install --download=foo.

[vweevers]

@isaacs SGTM. That approach (using env.npm_config_*) should also work on current npm, correct? And at least a few versions back. So in theory prebuild-install could make that change today.

[isaacs]

@isaacs SGTM. That approach (using env.npm_config_*) should also work on current npm, correct? And at least a few versions back. So in theory prebuild-install could make that change today.

A few yes...

That's been around since npm 0.1.8, landed a decade ago. npm/cli@821ca21

[vweevers]

On prebuild's side, it started here: prebuild/prebuild@a46eaa3. With a comment that is now answered, 5 years later:

// this is hackish - whats the correct way of doing this?

To be sure: has npm also always translated argv flags that npm itself doesn't use, e.g. npm install --build-from-source to env.npm_config_build_from_source? And what happens to negative boolean flags like --no-foobar? Thanks in advance for answering questions I imagine you've gotten a million times.

[isaacs]

To be sure: has npm also always translated argv flags that npm itself doesn't use, e.g. npm install --build-from-source to env.npm_config_build_from_source?

Not always, no. Just the last 10 years or so ;P But my guess is that install-prebuild won't work with npm 0.1.7 anyway, for a variety of other reasons.

And what happens to negative boolean flags like --no-foobar? Thanks in advance for answering questions I imagine you've gotten a million times.

$ npm run env --build-from-source | grep npm_config_build_from_source
npm_config_build_from_source=true

$ npm run env --foobar | grep npm_config_foobar
npm_config_foobar=true

$ npm run env --no-foobar | grep npm_config_foobar
npm_config_foobar=

$ npm run env --foobar=false | grep npm_config_foobar
npm_config_foobar=

Basically, false booleans just get set to an empty value in the environment. So you can check to see if it's either equal to 'true' for true, or '' for false.

[vweevers]

Does "put non-default configs in the env" also work for node-gyp?

For example, npm's default loglevel is "notice", but if that is no longer passed to node-gyp, then node-gyp will use the default of npmlog, which is "info".

[isaacs]

I don't maintain node-gyp, so I'm assuming it'll still do the same thing it's doing now.

But I think it's a good idea for npmlog to have the same default log level that npm has, so that should probably be updated anyway.

[vweevers]

I meant to suggest that someone should check node-gyp, not that that someone is you - although I did assume there's some coordination in place between these projects. If that's not the case, I'd be happy to open an issue over there and start the conversation (will probably be a short conversation, but better safe than sorry).

[darcyclarke]

@isaacs are we good to ratify this?