Isaacs/lifecycle envs #999

isaacs · 2020-03-10T20:36:56Z

Use @npmcli/run-script for the run-script command, and add a few missing configs to flatOptions.

mikemimik

Couple things I found:

small nits about using regexp
few questions around config we're setting in lib/config/flat-options.js
out of file scope cleanup in lib/npm.js
logical questions in lib/run-script.js

mikemimik · 2020-03-13T23:14:52Z

lib/run-script.js

-      }
-    }
-    cmds = [cmd]
+  const allScripts = scripts ? Object.keys(scripts) : {}


If scripts is truthy then you get an array of strings, if scripts is falsy you get an empty object. Seems like this should be the same value.

On line 146 we are using a for...of statement which is meant for array's and not objects (it will throw if it gets an object)

Confirmed, this is an oversight.

mikemimik · 2020-03-13T23:15:15Z

lib/run-script.js

+  const prefix = '  '
+  const cmds = []
+  const runScripts = []
+  for (const script of allScripts) {


Could be a problem (from comment above)

mikemimik · 2020-03-13T23:16:40Z

lib/run-script.js

+  const cmds = []
+  const runScripts = []
+  for (const script of allScripts) {
+    const list = cmdList.includes(script) ? cmds : runScripts


Both cmds and runScripts start as an empty array. I'm not sure why we need to check if cmdList has the script in it, if in both cases we return an empty array.

mikemimik · 2020-03-13T23:21:45Z

lib/run-script.js

+  for (const script of runScripts) {
+    output(prefix + script + indent + scripts[script])
+  }
+  return allScripts


The conditional if logic above is a little confusing. I now realise the point of the cmdList.includes() is to find out if what you typed as a user was, for example, npm run test. That output should be different than if you ran, for example, npm run myscript.

This is for listing out scripts that are runnable as a command differently from those that have to be run with npm run.

Eg:

$ node . run Lifecycle scripts included in npm: preversion bash scripts/update-authors.sh && git add AUTHORS && git commit -m "update AUTHORS" || true test npm run test-tap -- posttest npm run lint available via `npm run-script`: dumpconf env | grep npm | sort | uniq prepare node bin/npm-cli.js rebuild && node bin/npm-cli.js --no-audit --no-timing prune --prefix=. --no-global && rimraf test/*/*/node_modules && make -j4 mandocs ...

I think I understand. I guess the distinction wasn't clear when reading through the code. I'm not sure if that's indicative of me needing more context or the code not being clear enough.

When you run npm run without any args, it calls the list function, which dumps a list of which scripts are available.

The conditional on 147 is deciding which list to push the command name into, and then we loop over both lists to print out which ones are command lifecycles, and which have to be run explicitly.

mikemimik · 2020-03-13T23:56:21Z

lib/config/flat-options.js

@@ -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,


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

Can be literally anything.

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

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

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

mikemimik · 2020-03-13T23:57:37Z

lib/config/flat-options.js

@@ -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'),


Question: Are these the same thing?

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

lib/config/set-envs.js

mikemimik · 2020-03-14T00:12:28Z

lib/npm.js

@@ -113,19 +113,20 @@
            'See the README.md or bin/npm-cli.js for example usage.'


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?

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.

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

Last bit of #930

This sets environment variables for npm_command, npm_execpath, npm_node_execpath, NODE_OPTIONS, and any non-default configs that do not start with @, /, or _ (as those are potentially sensitive, or at least not useful). This is in preparation for the move from npm-lifecycle to @npmcli/run-script for running lifecycle scripts.

Setting up for the switch from npm-lifecycle to @npmcli/run-script.

The 'littleGuys' (ie, isntall, verison, udpate) are included in the cmdList.affordances section, so are no longer printed in the default help banner anyway. We can remove this variable.

…mmands

isaacs · 2020-03-20T00:41:45Z

Landed as part of #1021

isaacs requested a review from a team as a code owner March 10, 2020 20:36

isaacs changed the base branch from latest to release/v7.0.0-beta March 10, 2020 20:37

isaacs assigned mikemimik Mar 10, 2020

mikemimik added this to the OSS - Sprint 6 milestone Mar 10, 2020

mikemimik added Enhancement new feature or improvement Release 7.x work is associated with a specific npm 7 release semver:major backwards-incompatible breaking changes labels Mar 10, 2020

mikemimik reviewed Mar 14, 2020

View reviewed changes

isaacs added 10 commits March 17, 2020 10:00

Remove HTTP Referer header config

b2a7e16

Last bit of #930

nopt@4.0.3

391933e

@npmcli/run-script@1.3.0

a012888

Set non-default configs in the environment

2fb8c26

Setting up for the switch from npm-lifecycle to @npmcli/run-script.

Use @npmcli/run-script for npm run

6371f26

flatOptions: include preferDedupe and ignoreScripts

30ce3a9

fix tests for flatOptions with recent changes

b919531

Remove littleGuys, no longer relevant

c7dd395

The 'littleGuys' (ie, isntall, verison, udpate) are included in the cmdList.affordances section, so are no longer printed in the default help banner anyway. We can remove this variable.

Fix (with hacky temporary code in some cases) usage output for all co…

9cf75ed

…mmands

isaacs force-pushed the isaacs/lifecycle-envs branch from 251b751 to 9cf75ed Compare March 17, 2020 17:00

isaacs merged commit 9cf75ed into release/v7.0.0-beta Mar 20, 2020

isaacs unassigned mikemimik Mar 20, 2020

isaacs deleted the isaacs/lifecycle-envs branch October 2, 2020 21:41

nelsenpro mentioned this pull request Mar 8, 2024

[Snyk] Upgrade @sigstore/tuf from 2.3.0 to 2.3.1 nelsenpro/cli#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Isaacs/lifecycle envs #999

Isaacs/lifecycle envs #999

isaacs commented Mar 10, 2020

mikemimik left a comment

mikemimik Mar 13, 2020

isaacs Mar 14, 2020

mikemimik Mar 13, 2020

mikemimik Mar 13, 2020

mikemimik Mar 13, 2020

isaacs Mar 14, 2020

mikemimik Mar 16, 2020

isaacs Mar 17, 2020

mikemimik Mar 13, 2020

isaacs Mar 14, 2020

isaacs Mar 14, 2020

mikemimik Mar 13, 2020

isaacs Mar 14, 2020

mikemimik Mar 14, 2020

isaacs Mar 14, 2020

isaacs Mar 17, 2020

isaacs commented Mar 20, 2020

		@@ -113,19 +113,20 @@
		'See the README.md or bin/npm-cli.js for example usage.'

Isaacs/lifecycle envs #999

Isaacs/lifecycle envs #999

Conversation

isaacs commented Mar 10, 2020

mikemimik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaacs commented Mar 20, 2020