Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow running local binaries #36

Merged
merged 1 commit into from
Aug 27, 2024
Merged

Allow running local binaries #36

merged 1 commit into from
Aug 27, 2024

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented Aug 25, 2024

Mentioned in #14.

This PR allows running locally installed binaries.

@ehmicky ehmicky mentioned this pull request Aug 25, 2024
@sindresorhus
Copy link
Owner

Should it maybe be opt-in?

@ehmicky
Copy link
Collaborator Author

ehmicky commented Aug 26, 2024

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
Copy link
Owner

See sindresorhus/execa#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
Copy link
Collaborator Author

ehmicky commented Aug 26, 2024

See sindresorhus/execa#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: sindresorhus/execa#196, and this one with git: sindresorhus/execa#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?

@ehmicky ehmicky force-pushed the local-bin branch 8 times, most recently from 35717ac to 856a7c0 Compare August 27, 2024 19:30
@sindresorhus
Copy link
Owner

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
Copy link
Collaborator Author

ehmicky commented Aug 27, 2024

Done. 👍

test('options.preferLocal true can pass arguments to local npm binaries, ,', testLocalBinary, ',');
test('options.preferLocal true can pass arguments to local npm binaries, space', testLocalBinary, ' ');
test('options.preferLocal true can pass arguments to local npm binaries, *', testLocalBinary, '*');
test('options.preferLocal true can pass arguments to local npm binaries, ?', testLocalBinary, '?');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't have to be this PR, but maybe pass in the repeatable part of the titles here as a variable to reduce duplication.

@sindresorhus sindresorhus merged commit 381a015 into main Aug 27, 2024
12 checks passed
@sindresorhus sindresorhus deleted the local-bin branch August 27, 2024 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants