Use @npmcli/run-script for exec, explore; add interactive exec #2202

isaacs · 2020-11-19T19:33:21Z

This removes all other arg/shell escaping mechanisms, as they are no
longer needed, and will be un-done by puka in @npmcli/run-script anyway.

Adds an interactive shell mode when npm exec is run without any
arguments, allowing users to interactively test out commands in an npm
script environment. Previously, this would do nothing, and just exit.

Prevent weird behavior from npm explore ../blah. explore now can
only be used to explore packages that are actually found in the
relevant node_modules folder.

References

ljharb · 2020-11-20T07:40:46Z

Can the npm exec shell "not do that" in a non-TTY?

darcyclarke

As per @ljharb's note, I think it would be good if we ensured this interactive mode wasn't something that could trip up CI environments... I've suggested two changes that should support that.

darcyclarke · 2020-11-24T18:03:22Z

lib/exec.js


  if (call && args.length)
    throw usage

  const pathArr = [...PATH]

+  // nothing to maybe install, skip the arborist dance
+  if (!call && !args.length && !packages.length) {


Suggested change

if (!call && !args.length && !packages.length) {

if (!call && !args.length && !packages.length && isTTY && !ciDetect()) {

darcyclarke · 2020-11-24T18:18:52Z

docs/content/commands/npm-exec.md

@@ -30,6 +33,10 @@ This command allows you to run an arbitrary command from an npm package
 (either one installed locally, or fetched remotely), in a similar context
 as running it via `npm run`.

+Run without positional arguments or `--call`, this allows you to
+interactively run commands in the same sort of shell environment that
+`package.json` scripts are run.


Suggested change

`package.json` scripts are run.

`package.json` scripts are run; Only available in TTY/non-CI environments.

darcyclarke · 2020-11-24T18:27:57Z

lib/exec.js

@@ -59,15 +60,14 @@ const ciDetect = require('@npmcli/ci-detect')
 const crypto = require('crypto')
 const pacote = require('pacote')
 const npa = require('npm-package-arg')
-const escapeArg = require('./utils/escape-arg.js')
 const fileExists = require('./utils/file-exists.js')
 const PATH = require('./utils/path.js')

 const cmd = (args, cb) => exec(args).then(() => cb()).catch(cb)


Remove the isTTY definition on line 197 & hoist it up.

Suggested change

const cmd = (args, cb) => exec(args).then(() => cb()).catch(cb)

const cmd = (args, cb) => exec(args).then(() => cb()).catch(cb)

const isTTY = process.stdin.isTTY && process.stdout.isTTY

isaacs · 2020-11-24T18:40:39Z

Hm, if stdin is not a TTY, then it just means it'll read from whatever it is, and close when it closes.

$ echo 'echo $npm_package_name' > cmd

$ node . exec < cmd

Entering npm script environment
Type 'exit' or ^D when finished

npm

But, yes, if it's CI and it is a TTY, we should not do that. And if it's not a TTY, we should probably not show the helpful help banner.

isaacs · 2020-11-24T18:45:27Z

So, wait, if it is in CI, and we abort, that means you can't run commands from stdin in CI, which seems like a suboptimal situation? What if you have a bunch of shell commands in a file, and want to run npm exec < file.sh in CI?

I think the only situation we should exit early in is if it's CI and a TTY, with a warning that we're doing so.

isaacs · 2020-11-24T20:19:15Z

Updated with the logic described. Also simplified some of the isTTY checking, since it's really only stdin that we care about there. (If someone pipes the command to | tee output.log, then we should still behave the same way, since it's input availability that is relevant when prompting, but if you cat commands.sh | npm exec, then it should Just Work, and not give you a surprise prompt.)

darcyclarke

✅ LGTM

This removes all other arg/shell escaping mechanisms, as they are no longer needed, and will be un-done by puka in @npmcli/run-script anyway. Adds an interactive shell mode when `npm exec` is run without any arguments, allowing users to interactively test out commands in an npm script environment. Previously, this would do nothing, and just exit. Prevent weird behavior from `npm explore ../blah`. `explore` now can _only_ be used to explore packages that are actually found in the relevant `node_modules` folder.

@isaacs

Credit: @isaacs PR-URL: #2202 Close: #2202 Reviewed-by: @darcyclarke

@isaacs

Credit: @isaacs PR-URL: #2202 Close: #2202 Reviewed-by: @darcyclarke PR-URL: #2202 Credit: @isaacs Close: #2202 Reviewed-by: @ruyadorno

@isaacs

Credit: @isaacs PR-URL: #2202 Close: #2202 Reviewed-by: @darcyclarke

isaacs requested a review from nlf November 19, 2020 19:33

isaacs requested a review from a team as a code owner November 19, 2020 19:33

isaacs force-pushed the isaacs/better-script-escaping branch 2 times, most recently from b93460b to ec904f5 Compare November 19, 2020 19:49

darcyclarke added Release 7.x work is associated with a specific npm 7 release release: next These items should be addressed in the next release semver:minor new backwards-compatible feature labels Nov 24, 2020

darcyclarke suggested changes Nov 24, 2020

View reviewed changes

darcyclarke reviewed Nov 24, 2020

View reviewed changes

darcyclarke added release: next These items should be addressed in the next release and removed release: next These items should be addressed in the next release labels Nov 27, 2020

darcyclarke approved these changes Dec 2, 2020

View reviewed changes

darcyclarke pushed a commit that referenced this pull request Dec 2, 2020

Do not run interactive exec in CI when a TTY

f864b7b

Credit: @isaacs PR-URL: #2202 Close: #2202 Reviewed-by: @darcyclarke

darcyclarke force-pushed the isaacs/better-script-escaping branch from 1df4876 to f864b7b Compare December 2, 2020 21:31

Do not run interactive exec in CI when a TTY

4f94d42

Credit: @isaacs PR-URL: #2202 Close: #2202 Reviewed-by: @darcyclarke PR-URL: #2202 Credit: @isaacs Close: #2202 Reviewed-by: @ruyadorno

ruyadorno changed the base branch from latest to release/v7.1.0 December 4, 2020 19:17

ruyadorno force-pushed the isaacs/better-script-escaping branch from f864b7b to 4f94d42 Compare December 4, 2020 19:17

ruyadorno merged commit 4f94d42 into release/v7.1.0 Dec 4, 2020

ruyadorno pushed a commit that referenced this pull request Dec 4, 2020

Do not run interactive exec in CI when a TTY

2a1192e

Credit: @isaacs PR-URL: #2202 Close: #2202 Reviewed-by: @darcyclarke

This was referenced Dec 4, 2020

Release/v7.1.0 #2287

Merged

deps: upgrade npm to 7.1.0 nodejs/node#36395

Closed

nlf deleted the isaacs/better-script-escaping branch March 28, 2022 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use @npmcli/run-script for exec, explore; add interactive exec #2202

Use @npmcli/run-script for exec, explore; add interactive exec #2202

isaacs commented Nov 19, 2020

ljharb commented Nov 20, 2020

darcyclarke left a comment

darcyclarke Nov 24, 2020 •

edited

Loading

darcyclarke Nov 24, 2020

darcyclarke Nov 24, 2020 •

edited

Loading

isaacs commented Nov 24, 2020

isaacs commented Nov 24, 2020

isaacs commented Nov 24, 2020

darcyclarke left a comment

	if (!call && !args.length && !packages.length) {
	if (!call && !args.length && !packages.length && isTTY && !ciDetect()) {

	`package.json` scripts are run.
	`package.json` scripts are run; Only available in TTY/non-CI environments.

	const cmd = (args, cb) => exec(args).then(() => cb()).catch(cb)
	const cmd = (args, cb) => exec(args).then(() => cb()).catch(cb)
	const isTTY = process.stdin.isTTY && process.stdout.isTTY

Use @npmcli/run-script for exec, explore; add interactive exec #2202

Use @npmcli/run-script for exec, explore; add interactive exec #2202

Conversation

isaacs commented Nov 19, 2020

References

ljharb commented Nov 20, 2020

darcyclarke left a comment

Choose a reason for hiding this comment

darcyclarke Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

darcyclarke Nov 24, 2020

Choose a reason for hiding this comment

darcyclarke Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

isaacs commented Nov 24, 2020

isaacs commented Nov 24, 2020

isaacs commented Nov 24, 2020

darcyclarke left a comment

Choose a reason for hiding this comment

darcyclarke Nov 24, 2020 •

edited

Loading

darcyclarke Nov 24, 2020 •

edited

Loading