-
-
Notifications
You must be signed in to change notification settings - Fork 227
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
Steal some ideas/syntax from google/zx #484
Comments
Thanks for suggesting this @devinrhode2, I agree with you.
That being said, Execa has a more generic purpose. It seems to me the perfect solution would be a separate module based on Execa but with a I am curious what @sindresorhus thinks of this. |
That's what I was thinking too FWIW - my basic "wrapper" I'm using so far looks like this: export const exec = (cmdString) => {
let result = execaCommandSync(cmdString, {
all: true,
env: { FORCE_COLOR: 'true' },
})
if (typeof result.all !== 'string') {
if (typeof result.stdout === 'string') {
return result.stdout.trim()
}
return result
} else {
return result.all.trim()
}
} It's far from a finished product, but just wanted to share |
Probably a nice way to avoid throwing errors but not totally suppress them would be to return: const [stdout, stderr] = exec(`...`) |
Thanks for sharing! Those are some personal thoughts on the design of What I like
|
A lot of GitHub stars does not necessarily mean developers love the project. It has a lot of curb appeal The thing that tipped me to execa was discovering a thread where git bash was not working, and the answer was to use wsl. Which basically means windows support is not a serious goal... |
FWIW, this is the simple wrapper I've been getting some pretty good mileage out of: import { execaCommandSync } from 'execa'
/** @type {(cmdString: string, debug?: 'debug') => [string | undefined, string | unknown]} */
export const exec = (cmdString, debug) => {
let exception
let result
try {
result = execaCommandSync(cmdString, {
env: {
// @ts-expect-error - not application code
...process.env,
FORCE_COLOR: 'true',
},
})
} catch (e) {
exception = e
}
if (debug === 'debug') {
console.log(result)
console.log(exception)
}
return [
result?.stdout,
// If falsey, provide exception. If no exception, give falsey value.
result?.stderr || exception || result?.stderr,
]
} Anyone who has used react hooks will be comfortable with returning a tuple, kind of inspired by Golang too I think anyone who has put real effort into execa, might not like this wrapper, it's probably limiting a lot of functionality, and there's probably a lot of edge cases it doesn't handle well. But, I'm just trying to make something a little bit more portable than a 100% bash script |
Have one more small tweak for my |
I too am a fan of the I wanted to share a similar tagged templates API I've been exploring that feels like a nice balance of both worlds: Basicimport { $ } from 'execa';
const { stdout } = await $`echo foo`;
console.log(stdout); With optionsimport { $ } from 'execa';
await $({ stdio: "inherit" })`echo bar`; With pre-defined optionsimport { $ as base$ } from 'execa';
const $ = (templates, ...expressions) => base$({
stdio: "inherit",
shell: true,
})(templates, ...expressions);
await $`echo baz | sed 's/baz/qux/'`; If there is interest in using this tagged templates API I would be happy to contribute a PR. That said, curious what folks think!? |
Update: I've since ported the above API to a new package: https://www.npmjs.com/package/execa-extra @sindresorhus If you're interested in having me contribute a version of this API back to |
The danger with using tagged template literal is that a user expect to be able to interpolate anywhere in the string and have escaping correctly handled. I don't think you handle this. |
Other than that, I would personally be fine with including something like this to make it nicer to create scripts with @ehmicky Thoughts? |
Yes, I think this is a nice idea in general. EscapingEscaping is one of the most common sources of confusion with process execution. We have closed many non-issues related to that topic. Using template strings like zx does is an elegant way to isolate each argument (like quote strings in shell) without requiring the many pitfalls of shell escaping.
command`echo 'Hello world'` Instead of: command`echo ${'Hello world'}` zx also allows passing an array of arguments, which is nice too: command`echo ${['Hello', 'world']}` OptionsI like @aaronccasanova's usage of function binding as a nice way to allow passing options while still using template strings. command({ stdio: 'inherit' })`echo 'Hello world'` It also allows to change options for many commands: const shellCommand = command({ shell: true })
shellCommand`echo 'Hello world'` Which is a more functional way to achieve what zx does by requiring users to modify global objects like $.shell = '/usr/bin/bash' Function nameI would personally call this function Verbose modeAdding an option (like zx) to Execa to print commands as they execute would also be helpful in that context. |
Is whitespace escaping enough though? Just want to make sure we don't miss any injection cases. const foo = "'Hello'";
command`echo '${foo} world'`;
👍 |
Here's a draft PR so we have an
I assume so since my starting point was duplicating
I naively updated the implementation to accept an array and join elements with a
+1 |
I just realized the wrapper only accounts for the async API.. In an effort to use $({ type: 'sync' })`echo 'Hello world'` |
$.sync`echo 'Hello world'` ? |
Thank you so much @aaronccasanova for the draft PR! ❤️ I really like the minimalist approach of this.
What are your thoughts on relying on template strings' In other words, instead of: $`echo 'Hello world'` Users would escape by doing: $`echo ${'Hello world'}` While this does require a few more characters, this relies on neither whitespaces nor quotes to escape, which avoids many security problems and sources of confusion. Additionally, we could re-use the idea from |
Thanks @ehmicky! Regarding escaping/quoting, mind providing more examples (or perhaps there are already test cases you can point me to)? I'm struggling to understand the difference between the proposed Breaking down @sindresorhus' original example: const foo = "'Hello'";
$`echo '${foo} world'`;
execaCommand(`echo '${foo} world'`) // or
execaCommand(`echo ''Hello' world'`)
//=> ''Hello' world' |
Sorry for the confusion! Let me clarify what I mean.
Essentially, the idea is this: whitespaces separate arguments, unless escaped with $`echo Hello world` -> execa('echo', ['Hello', 'world'])
$`echo ${'Hello world'}` -> execa('echo', ['Hello world']) Which means that quotes and backslashes do not need to be interpreted as escaping characters, since $`echo "Hello world"` -> execa('echo', ['"Hello', 'world"'])
$`echo ${'"Hello world"'}` -> execa('echo', ['"Hello world"'])
$`echo Hello\\ world` -> execa('echo', ['Hello\\', 'world']) One of the reasons is that if a user passes: const foo = '"Hello world"';
$`echo ${foo}`; They would expect Another reason is that, in my experience, escaping characters (like quotes, backslashes, etc.) can lead to some issues and confusion. It also create some user expectation that whichever escaping syntax their shell is using should be supported: What are your thoughts on this? |
Incredible break down @ehmicky. That was a perfect primer for me to gain more context and form an opinion. I should preface my thoughts/opinions by stating that my integrations with That said, I took some time to investigate how |
Thanks for providing the links to One important issue with this approach is that this is Bash-specific. This will break for users of shells which do not support the Another issue is that it requires a shell to interpret those escape characters. By default, Execa does not run commands in shells, since it is more secure and also faster. Some users like the issue's initial poster @devinrhode2 are also mentioning being sometimes confused by
In my experience, quoting is a difficult problem to correctly solve. At first glance, it seems simple, but then edge cases come in, different shells, different versions of the same shell, etc. The additional complexity can leave some room for security bugs and potential injection. When we first implemented To avoid any of those issues, both Node.js
The above approach using |
@ehmicky the different shells/versions issue could be reduced. Require latest version of zsh available through Helps everyone. People writing scripts like myself have any easier time, because everyone running the script is required to have the same version. Makes code for execa extras easier to write, modify and maintain. Helps people running scripts know their shell is supported. |
Execa is not currently shell-specific. Requiring a specific shell to use specific features might be a problem to many users, including myself (I am not using @sindresorhus What are your thoughts on this? |
@ehmicky could we write some some sort of environment/safety/security check? It would run once whenever shell version changes
|
Execa will remain shell-agnostic. |
This is outside the scope of Execa. |
This is current in progress at #510 thanks to @aaronccasanova's great work! 👏 |
Released in |
I think zx has a very sexy syntax:
However, lots of little things about it I find confusing. (quoting behavior, weird dollar signs added into final commands:
git add $'.*ignore'
, etc. I just want it to run the string that I give it, no modifications at all.)I wonder if execa were to try and produce a similarly seemingly-simple "sexy"
$
function what would it be?This is more of a discussion, not an issue per se.
The text was updated successfully, but these errors were encountered: