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

Geting directly stdout via $cmd #1

Open
tomoemon opened this issue Jun 8, 2021 · 18 comments
Open

Geting directly stdout via $cmd #1

tomoemon opened this issue Jun 8, 2021 · 18 comments

Comments

@tomoemon
Copy link

tomoemon commented Jun 8, 2021

It is a very interesting product! I also love to use Deno & TypeScript.

I generally agree with the choice to make the default behavior a stream. However, I think the reason why zx is recognized as an alternative to bash is because the behavior of $cmd is close to that of bash as follows:

a="foo"
b=$(echo $foo | tr f F)
echo $b

Have you thought about supporting streams by providing options like $.rawcmd or $.streamcmd?

@Minigugus
Copy link
Owner

Minigugus commented Jun 8, 2021

First of all, thanks for the support :)

I originally planed to explain why I designed bazx this way in the README, but let's do this here.

I tried to focus bazx on features and performances instead of just copying the bash syntax, like zx do. I agree zx is really easy to play with but it also have some important limitations compared to bash: await $`yes`; for instance will always cause out of memory, since there is no way for zx to detect whether the result will be stored in a variable or not, and must therefore always collect the command ouput. With bash it's not a problem since the result is just piped to the terminal.

Also, from what I saw, some zx users are limited by the zx core design, for instance :

bazx tries to solves theses issues with:

  • No globals (more reliable, tree-shakable for bundling, enables writing library around bazx)
  • No platform-depend code in the surface API (Streams API instead of node stream, cross-platform piping)
  • No out-of-memory by default with streams (the user is responsible for collecting commands output if required)

The main idea is to stay close to bash while taking advantage of JavaScript features. This test for instance shows what is possible when we bring the best of both worlds (it may even be possible to pipe a WebSocket connection into commands easily for instance - not really useful but it's impossible to do with bash or zx).

Have you thought about supporting streams by providing options like $.raw`cmd`; or $.stream`cmd`;?

I don't want to surcharge the $ function, I prefer the user to bring their own $.raw if necessary.
And actually, $.raw is as simple as:

import { $, collect } from 'https://deno.land/x/bazx/mod.ts';

const $raw = async (...args: Parameters<typeof $>) => (await collect($(...args))).stdout;

const hello = await $raw`echo hello`;

$.stream is tricky since a command have both stdout and stderr as ouput. On the other hand, commands returned by $ already have .stdout and .stderr properties that are streams, so I don't think there is a need for $.stream.

Maybe you should take a look at the test folder, there are more example on how bazx is meant to be used

@tomoemon
Copy link
Author

tomoemon commented Jun 8, 2021

Thank you for detailed explanation!

I understand what you want to resolve.

The first thing I felt was that this writing style is a bit redundant.

let branch = (await stdout($`git branch`)).stdout

What I was thinking was that it would be better to be able to write more frequently used functions in a concise manner. Even if all the standard output is loaded to memory at once, we may not encounter such a process that results in out of memory very often. (it depends)

For easier scripting, it is easy to define my own function, such as $raw, that would load the standard output into memory in bulk, but I suspect that would result in the same problem. Therefore, my first suggestion was to make it possible to get the standard output with the $ function (more frequently used), and to get the stream with $.stream (or $.stdout/$.stderr/$.collect).

This discussion is based on the situation I consider when writing shell scripts, so it may be quite different from what you assume. Thanks

@Minigugus
Copy link
Owner

Ok, I see what you mean.

I had to do things like (await stdout($git branch)).stdout since only returning a string loose the exit code information 😕 In bash it's not a problem thanks to the special variable $?, but JavaScript is not designed in this way. Also, the original idea was to rely on destructuring to improve readability without loosing features:

let { code, stdout: branch, stderr: warnings }  = await collect($`git branch`))

But I agree it's not really efficient for the common use case. I thought about it and I realized the standard Response API has kind of the same features and issues than commands (a status code and a stream), so that maybe I can implements a similar interface and get $`cmd`.text(), $`cmd`.arrayBuffer(), and so on:

let branch = await $`git branch`.text()

This way I can split the API in 2 parts:

  • A minimalist but powerfull API for library and advanced users
function exec(cmd: [string, ...string[]], streams?: {
 stdin: ReadableStream<Uint8Array>,
 stdout: WritableStream<Uint8Array>,
 stderr: WritableStream<Uint8Array>
}): Promise<{ code: number }>
  • A "syntaxic sugar" API around the previous API (with $ and so on)
let b = await $`echo ${foo}`.pipe($`tr f F`).text()

I like this compromise since users that wants their own features don't need to import everything, while users that just wants to keep code simple and close to bash don't have to bring their own wrappers everywhere.

What do you think? Does it fit your needs?
Thanks for your feedback, it helps a lot 🙂

@tomoemon
Copy link
Author

tomoemon commented Jun 9, 2021

Looks great! Thank you for your understanding.

There are a few things I would like to confirm.

  • Are text() and arrayBuffer() supposed to return just the standard output? (I think that's fine)
  • If you make the interface similar to the Response API, what properties do you plan to support? (I was thinking "ok", "json", "blob" would be good to support too)
  • I can't immediately come up with a good design, but I would like to make the error handling a little easier. grep returns statuscode=1 when the target row is not found, but in Javascript, the filter function only returns an array of zero rows when no element is found, and no error is returned.
$ cat sample.ts
import { $, stdout } from 'https://deno.land/x/bazx/mod.ts';

await $`ls -1`.pipe($`grep hoge`);

$ deno run --allow-run sample.ts
$ ls -1 | grep hoge
error: Uncaught (in promise) Error: Command "ls -1 | grep hoge" exited with code 1
        throw new Error(`Command "${this}" exited with code ${res.code}`);
              ^
    at https://deno.land/x/[email protected]/src/bazx.ts:45:15
    at async file:///.../sample.ts:3:1

Thanks

@Minigugus
Copy link
Owner

Minigugus commented Jun 9, 2021

I would like to make the error handling a little easier.

Actually the fact that non-0 status code throws is intended to be configurable, but configuration is not implemented yet 😅 But as you said, errors handling is complicated here since a boolean in the configuration would disable errors for every commands, whereas users may only want to disable errors for grep for instance.

Another solution might be to let the user provide a predicate that check whether a specific command and status code should throw, like for instance:

options.throwsIf = (code: number, command: string) => (command === 'grep' && code === 1 ? false : code !== 0)

It's not really straightforward, but I don't find any other way to deal with this situation 😕

Are text() and arrayBuffer() supposed to return just the standard output?

I would say by default yes. The standard error output is supposed to report issues or at least to provide visual information to the user that shouldn't be directly processed by a program. However some users may want to process stderr, but for now I think it's not the main use case, and I don't want to complicate the surface API much more. (I guess that's also what you meant when you said (I think that's fine) ?)

If you make the interface similar to the Response API, what properties do you plan to support? (I was thinking "ok", "json", "blob" would be good to support too)

I think it could be possible to support all of them (only properties and functions, not the whole specification about behaviors), even if some properties like statusText would always return "" (maybe it could filled with an OS error description, for instance exit code 5 on Windows often refer to an "Acces denied" error ?).

However I haven't decided yet whether $`cmd`; should return directly a Response or { stdout: Response, stderr: Response, ... } with lazy initialization, so that is solve the standard error problem 😕

@tomoemon
Copy link
Author

tomoemon commented Jun 9, 2021

(I guess that's also what you meant when you said (I think that's fine) ?)

Sorry for the vague wording. I agree with you.

It's not really straightforward

Regarding to error handling, I'll try to read bazx code and think about another solution.

However I haven't decided yet whether $cmd; should return directly ...

There are not many cases where I want to handle stderr output, so I thought exec would be sufficient for my use case. But it would be better to make it possible to make my own function which can handle stderr via Response like API.

It's just an idea. It might be a bit tricky I think.

interface Response {
    text: () => string
    ok: boolean
    // ...
}

type CommandResponse = Response | {
    // ...
    stderr: Response;
}

@Minigugus
Copy link
Owner

Minigugus commented Jun 9, 2021

Regarding to error handling, I'll try to read bazx code and think about another solution.

Nice 👍 Currently, errors are thrown here:

bazx/src/bazx.ts

Lines 42 to 48 in 25ded30

this.#completed = this[RUN](this.#context.exec, streams).then(res => {
if (res.success || !this.#context.throw)
return res;
throw new Error(`Command "${this}" exited with code ${res.code}`);
}, err => {
throw new Error(`Command "${this}" failed: ${err?.message ?? err}`);
});

I couldn't find the time to add a custom Error subclass with command status info (like zx do), so if you want to contribute, you're welcome 😉

It's just an idea. It might be a bit tricky I think.

That's an interesting idea (Response & { stderr: Response }), but won't this be disruptive to users? Should the command status be passed to stderr as well?

What about:

interface Command extends Response {
    stdout: Body;
    stderr: Body;
}

Since Response implements Body, all Body-related method/getters in Command would be forwarded to the stdout property. It's easier to reason about and preserve the Response and Body semantics.

The only issue with Response and HTTP status vs command exit code is that the HTTP status code is known before the response body is available, whereas stdout/stderr are no more available when a command exited. Therefore, $ cannot return a Response with a valid status AND valid stdout/stderr. Moreover, headers, type, or even trailer would always be empty, so maybe a Command should extends directly Body instead of Response?

interface Command extends Body, PromiseLike<{ ok: boolean, status: number }> {
    cmd: [string, ...string[]];
    stdout: Body;
    stderr: Body;
}

// example
let cmd = $`echo Hello world!`
let [message, { status }] = await Promise.all([cmd.text(), cmd])

@tomoemon
Copy link
Author

tomoemon commented Jun 10, 2021

That's an interesting idea (Response & { stderr: Response }), but won't this be disruptive to users? Should the command status be passed to stderr as well?
What about:

Wow! That is just what I wanted to say! (Thank you for reading my mind)

The only issue with Response and HTTP status vs command exit code is that the HTTP status code is known before the response body is available, ...

Exactly. However, for simple APIs that don't support streams, I think it's possible to get stdout/stderr and status at the same time. zx's ProcessOutput does exactly that, allowing us to get all the values at the same time.

so maybe a Command should extends directly Body instead of Response?

Looks good to me.

BTW, when I looked at the code for zx, I noticed that one of the differences between zx and bazx is the way the child processes are launched.

Since zx runs processes through a shell by default, it is possible to use bash notation in $ function. We can use bash notation such as grep foo || : to supress errors. In addition, there is a nothrow function.

As for me, it is better to have no dependency on bash (Getting stuck due to the problem of different versions of bash being used is very annoying), so the current form that directly starts the process is better. However, I'd still like to have some kind of simple error suppressing method.

@tomoemon
Copy link
Author

I couldn't find the time to add a custom Error subclass with command status info (like zx do)

Even though I have read zx code, I couldn't find the custom error subclass or something like special error handling in zx. It just seems to check if status equals to 0 or not. Could you please show me the code you mentioned?

@Minigugus
Copy link
Owner

Wow! That is just what I wanted to say! (Thank you for reading my mind)

Not on purpose ^^' Maybe we just share the same goal ;)

I think it's possible to get stdout/stderr and status at the same time. zx's ProcessOutput does exactly that, allowing us to get all the values at the same time.

Yes but it means collecting all commands output by default 😕

This gives me another idea:

interface Command extends
    Body, // redirected to stdout, methods throws if status != 0 or command executable isn't found
    PromiseLike<Response> { // returns property `stdout`

    /**
     * Executes the command without collecting outputs,
     * then returns its exit code
     * @throws only if the command executable is not found
     */
    status: Promise<number>;

    /**
     * Executes the command without collecting outputs,
     * then returns whether the command exited successfully or not
     * @throws only if the command executable is not found
     */
    ok: Promise<boolean>;
    
    /**
     * Executes the command while collecting stdout,
     * then returns a Response representing its exit code and collected data
     * @throws only if the command executable is not found
     */
    stdout: Promise<Response>;
    
    /**
     * Executes the command while collecting stderr,
     * then returns a Response representing its exit code and collected data
     * @throws only if the command executable is not found
     */
    stderr: Promise<Response>;
    
    /**
     * Execute the command while collecting both stdout and stderr (like `2>&1`),
     * then returns a Response representing its exit code and collected data
     * @throws only if the command executable is not found
     */
    combined: Promise<Response>;
    
    /** Creates a new Command that pipes `this` command to the `command` command */
    pipe(command: Command): Command;
}

// usage

let result: Response = await $`ls -1`.pipe($`grep hoge`); // collects data; do NOT throw if status != 0
if (result.ok) {
    console.log('Found:', await result.text()); // do NOT throw even if `result.status` != 0
}

if (await $`ls -1`.pipe($`grep hoge`).ok) { // runs `ls -1 | grep hoge` WITHOUT collecting output; do NOT throw if status != 0
    console.log('Found something');
}

const found = await  $`ls -1`.pipe($`grep hoge`).text(); // throw if exit code is not 0

// reusable commands
let cmd =  $`ls -1`.pipe($`grep hoge`); // do NOT spawn any process (it's like a command template)
await cmd.ok; // runs cmd (spawn processes and wait for them to exit)
await cmd.ok; // runs cmd again (processes are spawned again - it may return a difference value)

This way it's easy to disable outputs collection (with the .ok or .status suffix), easy to deal with errors depending on the use case and avoid cmd duplication since Command instance can be reused 😉 Also, it's not that hard to implement 🙂

However, I'd still like to have some kind of simple error suppressing method.

Does the above proposal meet your needs?

As for me, it is better to have no dependency on bash [...], so the current form that directly starts the process is better.

It's also easier to deal with in code (no injection issues, no OS specific cases to deal with, and so on), and users that still want a shell can simply use something like $`sh -c "echo Hello world! | tee README"`;.

Even though I have read zx code, I couldn't find the custom error subclass or something like special error handling in zx. It just seems to check if status equals to 0 or not. Could you please show me the code you mentioned?

https://github.com/google/zx/blob/41834646c901ce9647ab14ee8b9ffe1c9a581270/index.mjs#L183-L221

zx always returns an Error https://github.com/google/zx/blob/41834646c901ce9647ab14ee8b9ffe1c9a581270/index.mjs#L53-L57

Currently bazx is missing the exit code on errors (like https://github.com/google/zx/blob/41834646c901ce9647ab14ee8b9ffe1c9a581270/examples/basics.mjs#L36-L40)

@tomoemon
Copy link
Author

tomoemon commented Jun 10, 2021

Awesome! The example you provided looks close to ideal interface.
But I haven't understood yet how you are going to implement error handling.

A

let result: Response = await $`ls -1`.pipe($`grep hoge`); // collects data; do NOT throw if status != 0
if (result.ok) {
    console.log('Found:', await result.text()); // do NOT throw even if `result.status` != 0
}

B

const found = await  $`ls -1`.pipe($`grep hoge`).text(); // throw if exit code is not 0

It is a little difficult to understand when an error is thrown (B) and when it is not (A).

zx always returns an Error

Thank you, I missed that ProcessOutput itself extends Error.

@Minigugus
Copy link
Owner

It is a little difficult to understand when an error is thrown (B) and when it is not (A).

The idea is that it depends on where the await is:

A

const found = await (await  $`ls -1`.pipe($`grep hoge`)).text(); // do NOT throw (`.text()` called on Response)

B

const found = await  $`ls -1`.pipe($`grep hoge`).text(); // do throw (`.text()` called on Command)

I agree it can be a bit confusing but I think it's a good compromise between API simplicity and feature richness.

Anyway, Response might not be a good idea finally:

> new Response([output], { status: exitCode })
Uncaught RangeError: The status provided (0) is outside the range [200, 599].
    at new Response (deno:extensions/fetch/23_response.js:238:15)

If I want to implement the Response API I'll have to override the status property 😞

@tomoemon
Copy link
Author

The idea is that it depends on where the await is:

Understood, thank you! I like this rule, so simple.

Anyway, Response might not be a good idea finally:

There may not be much need to be concerned about complying with the standard Response interface strictly. What we need are just ok and status.

@Minigugus
Copy link
Owner

Minigugus commented Jun 10, 2021

There may not be much need to be concerned about complying with the standard Response interface strictly. What we need are just ok and status.

I just wrote a proof-of-concept... It was simpler than expected 😄

Deno 1.10.2
exit using ctrl+d or close()
> let $ = (await import('./poc-response.ts')).$
undefined
> await $`grep -`
Response {
  body: ReadableStream { locked: false },
  bodyUsed: false,
  headers: Headers {},
  ok: false,
  redirected: false,
  status: 1,
  statusText: "",
  url: "grep -"
}
> let echo = await $`echo Hello world!`
undefined
> echo
Response {
  body: ReadableStream { locked: false },
  bodyUsed: false,
  headers: Headers {},
  ok: true,
  redirected: false,
  status: 0,
  statusText: "",
  url: "echo Hello world!"
}
> await echo.text()
"Hello world!\n"

EDIT: This poc is much more than $...

> let cat = await fetchBAXZ('cat', { body: 'Hello world!' }) // body is stdin... headers are environment variables ;)
undefined
> cat
Response {
  body: ReadableStream { locked: false },
  bodyUsed: false,
  headers: Headers {},
  ok: true,
  redirected: false,
  status: 0,
  statusText: "",
  url: "cat"
}
> await cat.text()
"Hello world!"

EDIT 2: Finally I rewrote everything from scratch, the new implementation is much better https://github.com/Minigugus/bazx/tree/dev

let response = await $`echo Hi!`;
let text = await $`echo Hello world!`.text()
let [json] = await $`echo ${JSON.stringify(["Hello world!"])}`.json()
let buffer = await $`echo Hello world!`
.pipe($`gzip`)
.arrayBuffer()

Also, I took the opportunity to add middlewares, I think it's a great addition to bazx

Minigugus added a commit that referenced this issue Jun 11, 2021
 * Add Response to the core (see #1)
 * Add middlewares supports
 * Add better examples
 * Drop old implementation
@tomoemon
Copy link
Author

Perfect! I was surprised at how quickly you implemented it!
I've tried dev code a few and it is very comfortable to use. I'll try to read details later!

@tomoemon
Copy link
Author

tomoemon commented Jun 11, 2021

(This might be another issue)
Common use-cases for me

reading line one by one

Opening file (or Piping another command) as stream and reading line by line are common use case, but Deno requires boring boilerplate codes.
https://decipher.dev/deno-by-example/advanced-readline/

I believe that it is better to have a utility function that can do this, something like following lineReaderFromStream. It would be better if this function was built into Command or CommandResponse.

import { readerFromStreamReader } from "https://deno.land/std/io/streams.ts";
import { BufReader } from "https://deno.land/std/io/bufio.ts";
import { TextProtoReader } from "https://deno.land/std/textproto/mod.ts";

const fetchRes = await fetch(
  "https://raw.githubusercontent.com/mayankchoubey/deno-doze/main/doze.ts",
);

for await (const line of lineReaderFromStream(fetchRes.body!)) {
  console.log(line);
}

function lineReaderFromStream(
  stream: ReadableStream<Uint8Array>,
): AsyncIterable<string> {
  const streamReader = readerFromStreamReader(stream.getReader());
  const bufReader = new BufReader(streamReader);
  const tp = new TextProtoReader(bufReader);
  return {
    async *[Symbol.asyncIterator]() {
      while (true) {
        const line = await tp.readLine();
        if (line == null) {
          break;
        }
        yield line;
      }
    },
  };
}

(EDIT 1) reading user input interactively

It would be better to provide simple function like zx's question.
https://github.com/google/zx#question

(EDIT 2) checking file conditions

Checking if a file exists via just [[ -e FILE ]] on bash.
More patterns in the following site. https://devhints.io/bash

Some checking conditions can be done by Deno std library.
https://doc.deno.land/https/deno.land/[email protected]/fs/mod.ts

Should I manage to do all these cases using Deno directly?

@Minigugus
Copy link
Owner

Minigugus commented Jun 11, 2021

Deno requires boring boilerplate codes.

https://deno.land/[email protected]/io/bufio.ts#L701-L719 + https://deno.land/[email protected]/io/streams.ts#L74-L94 =

import { readLines, readerFromStreamReader } from 'https://deno.land/[email protected]/io/mod.ts';

import $ from 'https://raw.githubusercontent.com/Minigugus/bazx/71767d812c7f759d3160bc7f6834c0330876e4b7/deno.ts';

const lines = [];
for await (const line of readLines(readerFromStreamReader(
  $`curl ${'https://raw.githubusercontent.com/mayankchoubey/deno-doze/main/doze.ts'}`.body.getReader()
))) {
  lines.push(line);
}

console.log(lines);

(that's why I like Deno 🙂)

Should I manage to do all these cases using Deno directly?

zx is more like a real shell, whereas bazx is a small library that helps running commands in JavaScript/TypeScript. Also, bazx focus on zero dependencies in order to stay isomorphic as much as possible.

For theses reason, you should manage to do these case with the runtime you are using. However, nothing prevent you from writing your own package around bazx, quite the opposite 😉

@tomoemon
Copy link
Author

Understood!

So If I want to provide utility functions for me and other bazx users, I can just publish it in my own repository myself.

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

No branches or pull requests

2 participants