TypeScript: pipeStdout (and other pipeXxx) is optional property

[koshic]

What is the reason to keep it optional?

pipeStdout?<Target extends ExecaChildPromise<StdoutStderrAll>>(target: Target): Target;
pipeStdout?(target: WritableStream | string): ExecaChildProcess<StdoutStderrType>;

It can't be user without extra check or '!`:

[ehmicky]

Hi @koshic,

pipeStdout() is undefined if the stdout option is 'ignore', 'ipc', a file descriptor integer, or a stream. Default value is 'pipe'.
The same applies to pipeStderr(), but using the stderr option.

pipeAll() is undefined if either:

• Both the stdout and stderr options are 'ignore', 'ipc', a file descriptor integer, or a stream.
• The all option is not true (default value is false).

execa/lib/pipe.js

Lines 31 to 33 in 834e372

	if (spawned.stdout !== null) {
	spawned.pipeStdout = pipeToTarget.bind(undefined, spawned, 'stdout');
	}

Ideally the types would take this into account. Would you be interested in submitting a PR to fix them?

[koshic]

@ehmicky yeah, let me experiment a bit )

[koshic]

As you can see, if we will use current strategy - modify options via generic parameter, and provide all possible combinations as execa definitions, we have to multiply all definitions by 3 (new modifiers: stdout + stdin + all) which makes code unmaintainable:

I see only one way - keep modifiers for options, but produce exact ExecaChildProcess type by additional conditional type wrapper which will work like switch-case statement:

O extends { stdin: 'pipe' /* + default value check */} ? { pipeStdout: () => {} } : { }

@ehmicky what do you think about it?

[ehmicky]

Yes, I think what you're highlighting was the reason I did not implement stricter typing in the first place when adding those pipe methods.

I agree with you. I'm not sure if this is what you are describing, but instead of method overloading, type inference could be used. I.e. only define execa() once and let Options<EncodingType> be inferred from the parameters passed as input. Something like:

export function execa<EncodingType extends EncodingOption = DefaultEncodingOption>(
	file: string,
	arguments?: readonly string[],
    // EncodingType is now inferred based on the `options` passed as input
	options?: Options<EncodingType>
// Which is used to guess whether `result.stdout|stderr|all` is a `string` or a `Buffer`
): ExecaChildProcess<EncodingType extends BufferEncodingOption ? Buffer : string>; 

What do you think?

[koshic]

Yes, I mean the same pattern - define function signature once, and use smart generic types. So, I propose to split that task into 2:

First PR - to replace overloading with type inference. Not a breaking change, I believe.
Second PR - to extend generics with PipeXxx options. May be a breaking change due to pipeStdout will be not present in resulting type in some cases.

[ehmicky]

This sounds like a good plan! I do like breaking it down like this, it's going to be easier to review.

I actually believe that this might not be a breaking change, because the type will go from Function | undefined to either Function or undefined, depending on the option being passed, so it is actually stricter. That being said, I am planning on making an unrelated PR which will require a new major release, so we can always bundle it with it, to be safe.

Thanks for your help!

[ehmicky]

@koshic Would you be interested in doing the second PR discussed above?

[koshic]

@ehmicky yeah, I need a few days to recover from my illness.

[ehmicky]

Thanks @koshic. Take care! 👩‍⚕️

[ehmicky]

This has been just been fixed in Execa 9.0.0, with the new subprocess.pipe() method. Please see (and share!) the release post and the changelog.