sindresorhus / execa

Process execution for humans
MIT License
6.76k stars 214 forks source link

Steal some ideas/syntax from google/zx #484

Closed devinrhode2 closed 1 year ago

devinrhode2 commented 2 years ago

I think zx has a very sexy syntax:

await $`echo ${things}`

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.

ehmicky commented 2 years ago

Thanks for suggesting this @devinrhode2, I agree with you.

zx has a very nice developer experience for users who wish to run a sequence of commands.

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 zx-like interface.

I am curious what @sindresorhus thinks of this.

devinrhode2 commented 2 years ago

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

devinrhode2 commented 2 years ago

Probably a nice way to avoid throwing errors but not totally suppress them would be to return:

const [stdout, stderr] = exec(`...`)
ehmicky commented 2 years ago

Thanks for sharing!

Those are some personal thoughts on the design of zx. This is quite subjective, so please feel free to comment!

What I like

$ alias

The $ alias is short and nice.

Template strings

Using template strings instead of a regular function call is convenient.

One downside is requiring using either separate methods, function composition (like nothrow()) or global state (like $.shell) to modify options.

Passing array of arguments

Using the template strings feature to allow users to pass array of arguments while still properly escaping it is a nice idea.

Piping

The pipe() method is convenient and feels close to shell experience.

Verbose mode

An option to print commands as they execute is useful.

Remote scripts

This can be useful in some cases. However, it seems to me this should be a separate module which wraps node to download the URL then execute it.

Literate programming

Executing samples from Markdown files is useful too.

What I dislike

Binary usage

The choice of exposing zx through a binary, i.e. requiring a shabang and/or calling the script through zx, adds an additional layer in-between node and the script, which I am expecting might create some issues in more complex setups. It is convenient. However, it seems to me the programmatic usage is much more straightforward, arguably simpler, and more future-proof:

import { $ } from 'zx' 

Global variables

Instead of injecting global variables (__filename, etc.), which often creates many issues (linting, weird environments which monkey-patch global variables like Jest does, etc.), it seems to me those should just be also available programmatically:

import { currentFilename } from 'zx'

Core modules

Exposing core modules through zx also adds unnecessary layers. Users could just import those modules.

import os from 'os'
process.chdir('/tmp')
import { setTimeout } from 'timers/promises'

Userland modules

The same applies to userland modules like chalk, globby, node-fetch and fs-extra. I can see the benefits of "all-batteries-included" approaches: the user needs to make fewer choices and does not need to explicitly install nor import those modules. However, it also is less modular and add unnecessary layers of abstraction.

Escaping

As you mention, escaping is somewhat cumbersome and has some odd syntax and limitations. That being said, escaping is a complicated topic when taking into account security, performance and cross-platform interoperability.

Note

That being said, zx is hugely popular with 24K GitHub stars as of now. Part of it might be due to it being developed by Google. But it also says something about users really liking the developer experience. So please take my comments above with some salt, since they definitely seem to have to nail the developer experience on this project, and Execa could definitely learn a lot from zx.

devinrhode2 commented 2 years ago

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...

devinrhode2 commented 2 years ago

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

devinrhode2 commented 2 years ago

Have one more small tweak for my exec util: https://gist.github.com/devinrhode2/e1e36fdf7f9465ed3037eac581cc07a5 Going to post further tweaks to that gist.

aaronccasanova commented 1 year ago

$ alias

The $ alias is short and nice.

Template strings

Using template strings instead of a regular function call is convenient. One downside is requiring using either separate methods, function composition (like nothrow()) or global state (like $.shell) to modify options.

I too am a fan of the $ tagged templates API and yet find the external configuration of options slightly inconvenient.

I wanted to share a similar tagged templates API I've been exploring that feels like a nice balance of both worlds:

Basic

import { $ } from 'execa';

const { stdout } = await $`echo foo`;

console.log(stdout);

With options

import { $ } from 'execa';

await $({ stdio: "inherit" })`echo bar`;

With pre-defined options

import { $ 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!?

aaronccasanova commented 1 year ago

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 execa, I would be happy to deprecate my wrapper and transfer ownership to you đź‘Ť

sindresorhus commented 1 year ago

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.

sindresorhus commented 1 year ago

Other than that, I would personally be fine with including something like this to make it nicer to create scripts with execa.

@ehmicky Thoughts?

ehmicky commented 1 year ago

Yes, I think this is a nice idea in general.

Escaping

Escaping 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.

execa-extra handles it, but using execaCommand() instead of execa(). So it relies on the same whitespace escaping mechanism as execaCommand() instead of taking advantage of template strings for 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']}`

Options

I 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.

$.shell = '/usr/bin/bash'

Function name

I would personally call this function $ like zx. It's short and convenient.

Verbose mode

Adding an option (like zx) to Execa to print commands as they execute would also be helpful in that context.

sindresorhus commented 1 year ago

So it relies on the same whitespace escaping

Is whitespace escaping enough though? Just want to make sure we don't miss any injection cases.

const foo = "'Hello'";
command`echo '${foo} world'`;

I would personally call this function $ like zx. It's short and convenient.

:+1:

aaronccasanova commented 1 year ago

Here's a draft PR so we have an execa implementation to reference: https://github.com/sindresorhus/execa/pull/510

Is whitespace escaping enough though? Just want to make sure we don't miss any injection cases.

I assume so since my starting point was duplicating execaCommand, adding logic to join templates/expressions, and passing the results to the existing mechanics.

zx also allows passing an array of arguments, which is nice too:

command`echo ${['Hello', 'world']}`

I naively updated the implementation to accept an array and join elements with a ' ' delimiter. Not sure if they do anything beyond that, but happy to investigate further if we decide to proceed.

I would personally call this function $ like zx. It's short and convenient.

+1

aaronccasanova commented 1 year ago

I just realized the wrapper only accounts for the async API.. In an effort to use $ as the function name, what are folks thoughts on adding an option to signify sync | async (default: 'async')? e.g.

$({ type: 'sync' })`echo 'Hello world'`
sindresorhus commented 1 year ago
$.sync`echo 'Hello world'`

?

ehmicky commented 1 year ago

Thank you so much @aaronccasanova for the draft PR! :heart: I really like the minimalist approach of this.

Is whitespace escaping enough though? Just want to make sure we don't miss any injection cases.

What are your thoughts on relying on template strings' ${...} for escaping, as opposed to quoting characters?

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 zx to allow array of arguments: ${[...args]}.

aaronccasanova commented 1 year ago

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 $ API and execaCommand. Currently, the $ API parses the tagged template args to a string and then passes it to the same execaCommand internals.

Breaking down @sindresorhus' original example:

const foo = "'Hello'";
$`echo '${foo} world'`;

echo '${foo} world' essentially de-sugars to:

execaCommand(`echo '${foo} world'`) // or
execaCommand(`echo ''Hello' world'`)
//=> ''Hello' world'
ehmicky commented 1 year ago

Sorry for the confusion! Let me clarify what I mean.

execaCommand() itselfs de-sugars to execa(), once whitespace parsing/escaping has been performed. execa() itself just sends the arguments as is to child_process.spawn(), which itself sends them as is to the OS syscall. Let me give some examples using execa() instead of execaCommand() to remove the additional escaping layer that command introduces.

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 ${...} already fulfills escaping needs. They would be interpreted as regular characters instead.

$`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 foo to be "atomic", i.e. they would expect the space between Hello and world to be escaped.

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: $"...", $'...', double/quadruple backslashes, and so on. Sometimes this complexity also leads itself to security risks.

What are your thoughts on this?

aaronccasanova commented 1 year ago

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 execa thus far have been very rudimentary (making directories, calling git commands interpolating branches, etc.) so ultimately I will trust your judgement on what you feel are sensible/secure defaults.

That said, I took some time to investigate how zx handles escaping and came across this document stating: Quotes will be added automatically if needed. From what I can tell, this is accomplished by passing each tagged template expression to the substitute() and quotes() utilities. Notice the described behavior applies to standalone expressions and each element if provided an array. My opinion is that this default behavior seems sensible, straight forward to integrate/document, and a good option if it alleviates security concerns. Thoughts?

ehmicky commented 1 year ago

Thanks for providing the links to zx's approach, that's very interesting. :book:

One important issue with this approach is that this is Bash-specific. This will break for users of shells which do not support the $'...' syntax (such as sh and cmd.exe). Shells differ widely when it comes to escaping.

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 zx's escaping behavior.

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.)

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 execaCommand(), Execa has been (thankfully incorrectly) flagged by Snyk for this. It's definitely something we should watch out for.

To avoid any of those issues, both Node.js child_process core module and Execa currently take the following approach:

The above approach using ${...} follows that approach, taking advantage of JavaScript itself parsing the ${...} inside template strings, which avoids any need for shell escaping.

devinrhode2 commented 1 year ago

@ehmicky the different shells/versions issue could be reduced. Require latest version of zsh available through brew install zsh. Otherwise fail.

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.

ehmicky commented 1 year ago

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 zsh). Except if those features cannot be implemented without taking a specific shell into account. But in this instance, there are some shell-agnostic solutions available (like the one mentioned above).

@sindresorhus What are your thoughts on this?

devinrhode2 commented 1 year ago

@ehmicky could we write some some sort of environment/safety/security check?

It would run once whenever shell version changes

  1. Create a hash of shell type, shell version, os version, etc
  2. Check if that hash has already passed security checks (stored in ~/.safe-shells.txt)
  3. If it hasn’t passed security checks, run security checks
  4. If it passes, store hash in ~/.safe-shells.txt
  5. If it fails, exit with non zero status code
sindresorhus commented 1 year ago

Execa will remain shell-agnostic.

sindresorhus commented 1 year ago

could we write some some sort of environment/safety/security check?

This is outside the scope of Execa.

ehmicky commented 1 year ago

This is current in progress at #510 thanks to @aaronccasanova's great work! :clap:

ehmicky commented 1 year ago

Released in v7.1.0.