sindresorhus / nano-spawn

Tiny process execution for humans — a better child_process
MIT License
195 stars 4 forks source link

Allow running local binaries #36

Closed ehmicky closed 3 weeks ago

ehmicky commented 3 weeks ago

Mentioned in #14.

This PR allows running locally installed binaries.

sindresorhus commented 3 weeks ago

Should it maybe be opt-in?

ehmicky commented 3 weeks ago

Hum, I actually always wondered why it was opt-in in Execa?

The cons I see are rather minor. Performance-wise, the OS will do stat calls through more directories in the PATH. However, on my machine, this is <=1ms (depending on how much the OS parallelizes and caches those calls), which is nothing compared to the time needed to allocate resources for the new process, then to load the process runtime (can easily go >=10ms in my experience).

Also, it passes the full process.env as environment variables to the subprocess, instead of specifying nothing and inheriting it. This probably has a performance impact since the exec* OS syscall will have a bigger arguments list, but of an even smaller magnitude.

On my machine, I have had node_modules/.bin in my PATH (from my .bashrc) and have never encountered a practical issue. It's seemed to always work (at least for me).

The main pro is that executing binaries from your package.json dependencies is quite a common use case. Users might assume it to work out-of-the-box.

Also, not having an option would keep the API smaller. I am not sure why one would actually want to disable this feature?

Am I missing something? What are your thoughts on this?

sindresorhus commented 3 weeks ago

See https://github.com/sindresorhus/execa/pull/314 for context.

Preferring local binaries for user scripts definitely makes sense, but for reusable packages, it's not that commonly used. And it may even cause problems if you want to execute a global binary, and then a different version exists locally in node_modules, which can cause unpredictability. And because of hoisting and dependency resolution, it may not even reproduce every time.

I don't feel strongly about it though and could be convinced otherwise.

ehmicky commented 3 weeks ago

See https://github.com/sindresorhus/execa/pull/314 for context.

Thanks for pointing at this issue, my memory is bad! :)

I just read through this PR and the several related issues. In Execa, the preferLocal option used to both add process.execPath and node_modules/.bin to the PATH. In the above cases (including this issue with Yarn: https://github.com/sindresorhus/execa/issues/196, and this one with git: https://github.com/sindresorhus/execa/issues/153), users had some problems with adding process.execPath to the PATH, not node_modules/.bin.

Preferring local binaries for user scripts definitely makes sense, but for reusable packages, it's not that commonly used. And it may even cause problems if you want to execute a global binary, and then a different version exists locally in node_modules, which can cause unpredictability. And because of hoisting and dependency resolution, it may not even reproduce every time.

Very good point. Calling binaries in a re-usable Node module should work in most (but not all) cases, but it seems like this is a bad pattern for the reasons you are mentioning. Probably not something we should encourage.

On the other hand, it is probably always a good thing in user scripts or apps. But then the package size should not matter as much (except maybe in a serverless function) and users should prefer Execa instead.

I don't feel strongly about it though and could be convinced otherwise.

Should we expose this under a boolean option, defaulting to false? If so, how to call it? preferLocal or something else?

sindresorhus commented 3 weeks ago

Should we expose this under a boolean option, defaulting to false?

👍

If so, how to call it? preferLocal or something else?

Thought about it and couldn't come up with a better name, so yeah preferLocal, unless you have a better suggestion.

ehmicky commented 3 weeks ago

Done. :+1: