zkat / npx

execute npm package binaries (moved)
https://github.com/npm/npx
Other
2.63k stars 105 forks source link

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

Closed jdalton closed 6 years ago

jdalton commented 6 years ago

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 https://github.com/npm/npm/pull/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 commented 6 years ago

Any guidance on unit tests would be appreciated.

jdalton commented 6 years ago

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

zkat commented 6 years ago

@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 commented 6 years ago

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.

https://github.com/zkat/npx/blob/13fea318c7cf46ba24b4ad11fb88600fa5821e1d/index.js#L291 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 commented 6 years ago

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

zkat commented 6 years ago

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

noelleleigh commented 6 years ago

@zkat With my example, this condition doesn't get met: https://github.com/zkat/npx/blob/13fea318c7cf46ba24b4ad11fb88600fa5821e1d/index.js#L273 Which means when it comes time to run a command in a new process: https://github.com/zkat/npx/blob/13fea318c7cf46ba24b4ad11fb88600fa5821e1d/index.js#L291 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 commented 6 years ago

@noahleigh Does https://github.com/zkat/npx/pull/174 resolve the problem?

noelleleigh commented 6 years ago

@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