Add return promise to the Module.runMain() branch. #172

jdalton · 2018-04-12T05:28:51Z

This PR adds an in-process opt-in to avoid defaulting to running in the same process as npx. Running in process causes the promise to resolve quicker and creates problems with packages that tie-in to process.on('exit') or call process.exit(). Related to npm/npm#20303.

Update:

I can also flip this so instead of opting-in to in-process the user could opt-in to child process.

Update:

Flipped to adding a --always-spawn option.

jdalton · 2018-04-12T05:37:00Z

Any guidance on unit tests would be appreciated.

jdalton · 2018-04-12T05:48:10Z

index.js

+            }
+          })
+        })
+      })
    } else if (!existing && argv.nodeArg && argv.nodeArg.length) {


The setImmediate use here is just to get it off the synchronous beforeExit event loop.
It could be a setTimeout of 0 or process.nextTick.

I shifted to opting-in to running in the same process since that is prickly.

jdalton · 2018-04-12T07:01:24Z

test/index.js

  return child.spawn('node', [
    NPX_ESC, '--quiet', 'echo-cli', 'hewwo'
  ], {stdio: 'pipe'}).then(res => {
-    t.equal(res.stdout.trim(), 'hewwo')
+    t.equal(res.stdout.trim(), isWindows ? '' : 'hewwo')
    t.end()


Any ideas on why the res.stdout is empty on Windows?

it shouldn't have been? /cc @noahleigh

I think the switch to running in a child process in the current PR state has smth to do with it.

zkat · 2018-04-12T16:41:10Z

parse-args.js

@@ -208,6 +208,10 @@ function yargsParser (argv, defaultNpm) {
      describe: Y()`Ignores existing binaries in $PATH, or in the local project. This forces npx to do a temporary install and use the latest version.`,
      type: 'boolean'
    })
+    .option('in-process', {
+      describe: Y()`Allow the command to be executed in the same process as npx instead of a child process.`,
+      type: 'boolean'


in re opt-in, I think this should default to true -- that way, it won't be a breaking change, and this has been part of npx for a while, so I'd rather not start spawning processes here.

What do you think about flipping the new option to being child which enforces the child process branch? I kind of dig that approach since the in-process option is more of a suggestion if it's doable (no nodeArg or shell or other stuff) where the ~~child~~ --always-spawn option is explicit and can happen 💯of the time.

zkat · 2018-04-12T16:45:24Z

test/index.js

  return child.spawn('node', [
    NPX_ESC, '--quiet', 'echo-cli', 'hewwo'
  ], {stdio: 'pipe'}).then(res => {
-    t.equal(res.stdout.trim(), 'hewwo')
+    t.equal(res.stdout.trim(), isWindows ? '' : 'hewwo')
    t.end()


it shouldn't have been? /cc @noahleigh

…rocess.

zkat

👍 nice. This will be useful, specially for libnpx users. This feature was definitely optimized for plain npx usage.

jdalton · 2018-04-12T17:57:37Z

Thanks @zkat! Would you like a follow-up PR for a unit test?

zkat · 2018-04-12T17:58:26Z

@jdalton If you want, give it a shot! -- but if it's too tricky to write a test for, don't worry about it. I mainly want to unblock you right now though :)

noelleleigh · 2018-04-12T18:33:21Z

After pulling down these changes to my local repo on my Windows 10 machine, I tested it with:

node .\test\util\npx-bin.js --always-spawn echo-cli hewwo

After the packages were installed, it launched my text editor with the index.js file for echo-cli in the npx cache folder. hewwo was not echoed to my console.

npx/index.js

Line 291 in 13fea31

return child.runCommand(cmd, opts).catch(err => {

The cmd here is just a .js file with no mention of the node binary that lives in process.argv[0] to actually run it.

I assume this isn't the intended behavior?

zkat · 2018-04-12T18:41:08Z

No that's not intended. Ohno. I'll try and fix it once I've got my laptop out again.

zkat · 2018-04-12T18:54:56Z

actually I've no idea why this is happening, right now lol

noelleleigh · 2018-04-12T19:07:22Z

@zkat
With my example, this condition doesn't get met:

npx/index.js

Line 273 in 13fea31

if (existing && argv.nodeArg && argv.nodeArg.length) {

Which means when it comes time to run a command in a new process:

npx/index.js

Line 291 in 13fea31

return child.runCommand(cmd, opts).catch(err => {

cmd and opts look like:

> cmd
"C:\Users\nleigh\AppData\Roaming\npm-cache\_npx\14100\node_modules\echo-cli\dist\index.js"

> opts
{
    ....
    "cmdOpts": ["hewwo"],
    ....
}

I think they should look like this:

> cmd
"C:\Program Files\nodejs\node.exe"

> opts
{
    ...
    "cmdOpts": [
        "C:\Users\nleigh\AppData\Roaming\npm-cache\_npx\14100\node_modules\echo-cli\dist\index.js",
        "hewwo"
    ],
    ....
}

Which is what the code within that first condition block does (more or less)

jdalton · 2018-04-12T20:49:24Z

@noahleigh Does #174 resolve the problem?

noelleleigh · 2018-04-12T21:00:14Z

@jdalton After I apply the change I made in #173 , that PR works for me.

C:\Users\nleigh\dev\npx [pr/174 +1 ~1 -0 !]> node .\test\util\npx-bin.js --always-spawn echo-cli hewwo
npx: installed 47 in 3.353s
hewwo

@jdalton

add --always-spawn to opt out of process takeover optimization feature Credit: @jdalton PR-URL: zkat/npx#172

@jdalton

add --always-spawn to opt out of process takeover optimization feature Credit: @jdalton PR-URL: zkat/npx#172

jdalton mentioned this pull request Apr 12, 2018

feat: add npm init <create-pkg-name> command npm/npm#20303

Merged

jdalton commented Apr 12, 2018

View reviewed changes

zkat reviewed Apr 12, 2018

View reviewed changes

fix: add --always-spawn opt-in for executing the command in a child p…

0f55663

…rocess.

zkat approved these changes Apr 12, 2018

View reviewed changes

zkat merged commit c0d6abc into zkat:latest Apr 12, 2018

jdalton deleted the run-main branch April 12, 2018 17:57

jdalton mentioned this pull request Apr 12, 2018

fix: spawn child processes with node without relying on the shebang. #174

Merged

iarna added a commit to npm/npm that referenced this pull request Apr 12, 2018

libnpx@10.1.1

3c0bbcb

add --always-spawn to opt out of process takeover optimization feature Credit: @jdalton PR-URL: zkat/npx#172

iarna added a commit to npm/npm that referenced this pull request Apr 12, 2018

libnpx@10.1.1

2b57006

add --always-spawn to opt out of process takeover optimization feature Credit: @jdalton PR-URL: zkat/npx#172

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add return promise to the Module.runMain() branch. #172

Add return promise to the Module.runMain() branch. #172

jdalton commented Apr 12, 2018 •

edited

Loading

jdalton commented Apr 12, 2018

jdalton Apr 12, 2018 •

edited

Loading

jdalton Apr 12, 2018

jdalton Apr 12, 2018 •

edited

Loading

zkat Apr 12, 2018

jdalton Apr 12, 2018

zkat Apr 12, 2018

jdalton Apr 12, 2018 •

edited

Loading

zkat Apr 12, 2018

zkat left a comment

jdalton commented Apr 12, 2018

zkat commented Apr 12, 2018

noelleleigh commented Apr 12, 2018

zkat commented Apr 12, 2018

zkat commented Apr 12, 2018

noelleleigh commented Apr 12, 2018

jdalton commented Apr 12, 2018

noelleleigh commented Apr 12, 2018

Add return promise to the Module.runMain() branch. #172

Add return promise to the Module.runMain() branch. #172

Conversation

jdalton commented Apr 12, 2018 • edited Loading

jdalton commented Apr 12, 2018

jdalton Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

jdalton Apr 12, 2018

Choose a reason for hiding this comment

jdalton Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

zkat Apr 12, 2018

Choose a reason for hiding this comment

jdalton Apr 12, 2018

Choose a reason for hiding this comment

zkat Apr 12, 2018

Choose a reason for hiding this comment

jdalton Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

zkat Apr 12, 2018

Choose a reason for hiding this comment

zkat left a comment

Choose a reason for hiding this comment

jdalton commented Apr 12, 2018

zkat commented Apr 12, 2018

noelleleigh commented Apr 12, 2018

zkat commented Apr 12, 2018

zkat commented Apr 12, 2018

noelleleigh commented Apr 12, 2018

jdalton commented Apr 12, 2018

noelleleigh commented Apr 12, 2018

jdalton commented Apr 12, 2018 •

edited

Loading

jdalton Apr 12, 2018 •

edited

Loading

jdalton Apr 12, 2018 •

edited

Loading

jdalton Apr 12, 2018 •

edited

Loading