tinylibs / tinyexec

📟 A tiny, higher level interface around child_process
MIT License
100 stars 1 forks source link

Technical design/interface #1

Closed 43081j closed 2 months ago

43081j commented 4 months ago

Before we decide if this is a good idea or not, we need to agree on the interface and what we do/don't support (to keep it "tiny").

If it turns out there's no real way to make a utility like this tiny, we should just abandon the repo I think.

Proposed usage

Borrowing some patterns from execa but not all, this is the proposed usage I have:

// Ability to iterate over stdout lines
for await (const line of exec('cmd')) {
  console.log(line);
}

// Ability to get the entirety of stdout/stderr
// Would return `{stderr, stdout}`
console.log(
  await exec('cmd')
);

// Ability to get the entirety of stdout/stderr while piping
// Would return `{stderr, stdout}`
console.log(
  await exec('cmd')
    .pipe('cmd2')
    .pipe('cmd3')
);

Proposed interface

That suggests we need the interface to look like this (roughly):

// Main export, a function
type TinyExec = (
  command: string,
  args?: string[],
  options?: Options
) => Result;

// Return type of the main function
// It is a promise which, when resolved, contains the stdout/stderr
// Before resolution, it has additional methods
type Result = PromiseLike<Output> & OutputApi;

// Additional methods the main function's result has
interface OutputApi {
  // Pipe stdout to another command
  pipe(command: string, args?: string[], options?: PipeOptions): Result;

  // Kill the process
  kill(signal: number): boolean;

  // The underlying ChildProcess
  process: ChildProcess;

  // Ease of use getters
  get pid(): number;
  get cancelled(): boolean; // If the process was cancelled by us (AbortSignal)
  get killed(): boolean; // If the process was killed by us
}

Open questions on the interface

Which properties of ChildProcess do we want to expose directly, if any?

Things like killed are actually just pass throughs to the underlying process to save a few words.

Do we want to expose common members or just tell consumers to access process directly?

Which inputs/states do we want to expose?

For example:

What options do we want to provide?

So far, I had:

interface Options {
  // Signal to abort the process
  signal: AbortSignal;

  // Define the stdin of the process we launch
  // Not sure what type this would be
  stdin: ReadbleStream | Result;

  // Whether the sub process should continue running after we exit our script or not
  persist: boolean;

  // Timeout to apply (i.e. kill the sub process when we reach this time)
  timeout: number;

  // Pass through to node's own `ChildProcess` options
  nodeOptions: ChildProcessOptions;
}

Requirements

The suggested requirements:

What this is not for

There are some things I don't think we should implement, to keep things simple.

cc @Aslemammad

Aslemammad commented 4 months ago

You nailed it, I think I've just witnessed the best API design so far.

Ok one note, like tinybench, maybe we can add support for addEventListener also? https://github.com/tinylibs/tinybench/blob/main/src/bench.ts#L20

Second note, how is process support in other environments? I agree with you, the more we fallback to native solutions, the better. So I like the process part.

toFile I think it's common, but we're tiny, so we might not need it? What can be done is just show an example of how we write streams in a nodejs project, which fairly easy, but shows the users that they have control instead.

stdin: unknown;

Maybe if we can have a unified type, it'd be even better? like it accepts streams, and if the users want to pass strings directly, we might provide them with toStream('str')? I'm not sure about this one, let me know.

killOnExit: boolean;

This is not clear to me!

43081j commented 4 months ago

Ok one note, like tinybench, maybe we can add support for addEventListener also?

i think this makes sense. it would've been my go to too.

Second note, how is process support in other environments? I agree with you, the more we fallback to native solutions, the better. So I like the process part.

basically you can see here:

https://nodejs.org/api/child_process.html#class-childprocess

we already have access to a few things we wanted (exitCode, kill(signal), killed and pid in particular).

if we offered these at top level, it just means we'd have a pass-through on the result (i.e. once you await it):

get exitCode() {
  return this.process.exitCode;
}

might be nice for usability but i've no preference. though if we did choose to do this, we'd have to draw the line somewhere i think to avoid ending up with a huge interface.

toFile I think it's common, but we're tiny, so we might not need it? What can be done is just show an example of how we write streams in a nodejs project, which fairly easy, but shows the users that they have control instead.

this was my thinking too. just document how to do it via a stream basically, and we don't need to implement it ourselves then

Maybe if we can have a unified type, it'd be even better? like it accepts streams, and if the users want to pass strings directly, we might provide them with toStream('str')? I'm not sure about this one, let me know.

i think so yeah, maybe we just accept streams and other tinyexec results

This is not clear to me!

basically an option to tell us to kill the sub process when our process exits. normally if you spawn a subprocess, then exit your script, it will carry on iirc.

on a side note, for the Options, i think it makes sense to have something like {nodeOptions: ChildProcessOptions} so we can pass through any underlying node options (from here).

i'll update the post with the ones we agree on too 👍

Aslemammad commented 4 months ago

basically you can see here:

Oh yea, that part was clear to me, but I was asking how the support would be for other envs like deno or bun.

And let's not infer and just give the users the process object (if that does not cause issue for being cross environment)

basically an option to tell us to kill the sub process when our process exits. normally if you spawn a subprocess, then exit your script, it will carry on iirc.

Oh amazing, can't we have a better/simpler name?

on a side note, for the Options, i think it makes sense to have something like {nodeOptions: ChildProcessOptions} so we can pass through any underlying node options (from here).

Agreed, just curious how we'd have denoOptions, bunOptions, ...

AriPerkkio commented 4 months ago

Overall I like the idea. :+1:

Though what's the main motivation for this package? Why choose this over execa? Smaller API?

// Return type of the main function
// It is a promise which, when resolved, contains the stdout/stderr
// Before resolution, it has additional methods
type Result = PromiseLike<Output> & OutputApi;

Would resolving the promise returned by exec() kill the process as well? Or does user have to manually kill each process? In long-running applications this would be important.

What options do we want to provide?

How about timeout option? Sometimes the commands can get stuck and it would be nice to have built-in mechanism for killing the process after specific delay.

killOnExit: boolean;

This is not clear to me!

This is great to have built-in. Nested child_process's will leave zombie processes if not cleaned manually: https://nodejs.org/api/child_process.html#subprocesskillsignal

On Linux, child processes of child processes will not be terminated when attempting to kill their parent.

43081j commented 4 months ago

Oh amazing, can't we have a better/simpler name?

i wonder if we should call it persist: boolean, since the default will be to not persist (i.e. persist: false)

if someone sets persist: true, we just leave the process running after we exit

Agreed, just curious how we'd have denoOptions, bunOptions

i get you now.

for deno at least, they have node interop so you can use node:fs etc with the same interface as node itself. so we should be ok on that front, though im not too sure about bun

Though what's the main motivation for this package? Why choose this over execa? Smaller API?

primarily to have a smaller package, but also to provide a more concise API (think rollup vs webpack being this vs execa).

execa is 301kb according to pkg-size and 17 packages.

Would resolving the promise returned by exec() kill the process as well? Or does user have to manually kill each process? In long-running applications this would be important.

Awaiting it would wait until the process exits (or times out, errors, etc). so we can give the entire stdout to the user.

if it never exits, it would get timed out by default i think. then if you disable the timeout, it would wait for the user to kill it manually.

How about timeout option? Sometimes the commands can get stuck and it would be nice to have built-in mechanism for killing the process after specific delay

a timeout option makes sense, plus the underlying child_process already takes such an option so we can just pass it through directly

Aslemammad commented 4 months ago

i wonder if we should call it persist: boolean, since the default will be to not persist (i.e. persist: false)

With a jsdoc comment, that can work!

Awaiting it would wait until the process exits (or times out, errors, etc). so we can give the entire stdout to the user.

Nice!

a timeout option makes sense, plus the underlying child_process already takes such an option so we can just pass it through directly

This is so helpful!

43081j commented 3 months ago

one interesting thing i noticed is that escaping should happen automatically.. 🤔

basically cross-spawn and execa seem to do a bunch of redundant stuff. both do not escape when shell is set. when it isn't set, node escapes parameters anyway afaict.

so this means the only situation where execa escapes right now is when you don't pass shell but you do run a windows binary (so under the hood, execa passes shell as cmd.exe). i don't think we need to care, i think we leave it up to the consumer

43081j commented 2 months ago

closing since we now have an alpha version in main