Duplex stream #424

sloonz · 2020-05-15T19:08:08Z

First shot at an implementation for fixes #143

process end event may come before stdin/stdout streams flush

sindresorhus · 2020-05-16T09:13:17Z

test/duplex.js

+	failingStream.destroy(new Error('oops'));
+	const {message} = await t.throwsAsync(pipeline(failingStream, duplex, makeCollector()));
+	t.is(message, 'oops');
+	const err = await t.throwsAsync(duplex);


err => error

sindresorhus · 2020-05-16T09:13:39Z

Thanks for working on this 🙌

Make sure you add a lot of tests.

jschatz1 · 2020-05-17T00:43:15Z

Do you need to implement writev as well?

https://nodejs.org/api/stream.html#stream_api_for_stream_implementers

sloonz · 2020-05-17T20:56:28Z

No need to implement writev, it's optional method that is here for optimization purposes if you can handle multiple chunks at once, which we can't since we just pass them to process stdin.

Do you have an idea on what other tests I could make ?

Unless someone can lend me a hand, it will take some times for me to track the test failures on windows ; I don't have access to a windows machine right now.

sloonz · 2020-05-21T17:28:15Z

Okay, found why some tests where failing on Windows and not Linux, but that raises more questions @sindresorhus

It’s on unit tests testing this code path : https://github.com/sloonz/execa/blob/28ac048159f6ea1fa2c27499451c014a74e80265/index.js#L268-L287

I tried to trigger an exception on line 266 by passing null to file. This works as intended on Linux ; however, on Windows, the exception gets triggered earlier (at line 261, handleArgs), and the exception directly unwind to the caller instead of being caught and handled.

Two questions :

How can I consistently trigger an exception on childProcess.spawn ?
I have not tested, but my problem most likely is also here for the main execa() function, where execa(null) returns an error (which doubles as a rejected promise) on Linux whereas it just throws on Windows. Is this the intended behavoir ? Should I fill a bug for this ?

sloonz · 2020-05-26T15:58:49Z

I think the feature is in a good spot for review/possible integration in the current iteration ; should I squash the commits ?

sindresorhus · 2020-06-01T18:03:41Z

index.d.ts

@@ -544,6 +544,20 @@ declare const execa: {
 	): execa.ExecaChildProcess<Buffer>;
 	node(scriptPath: string, options?: execa.Options): execa.ExecaChildProcess;
 	node(scriptPath: string, options?: execa.Options<null>): execa.ExecaChildProcess<Buffer>;
+
+	/**
+	Execute a file as a transform stream


Suggested change

Execute a file as a transform stream

Execute a file as a transform stream.

And it's not a transform stream.

sindresorhus · 2020-06-01T18:03:41Z

index.js

+		result.then(() => {}).catch(error => error),
+		new Promise((resolve, _) => stdin.on('close', resolve)),
+		new Promise((resolve, _) => output.on('close', resolve))
+	]).then(([error]) => {


Use await.

sindresorhus · 2020-06-01T18:03:41Z

index.js

+	});
+
+	Promise.all([
+		result.then(() => {}).catch(error => error),


Do you really need .then here?

Yes, otherwise non-error result will be resolved by the complete promise. A more explicit intent would be

result.then(() => { return null; }).catch(error => error)

But anyway this is changed by the use of await instead of then.

sindresorhus · 2020-06-01T18:03:41Z

index.js

+		}
+	});
+
+	const result = processDone.then(({error, exitCode, signal, timedOut}) => {


Use await.

sindresorhus · 2020-06-01T18:03:41Z

test/duplex.js

+
+function makeCollector() {
+	const chunks = [];
+	const w = new stream.Writable({


Use descriptive variable names.

sindresorhus · 2020-06-01T18:03:41Z

test/duplex.js

+function makeCollector() {
+	const chunks = [];
+	const w = new stream.Writable({
+		write(chunk, encoding, cb) {


Suggested change

write(chunk, encoding, cb) {

write(chunk, encoding, callback) {

sindresorhus · 2020-06-01T18:03:41Z

test/duplex.js

+	const duplex = execa.duplexStream('forever');
+	const failingStream = makeEmptyReadableStream();
+	failingStream.destroy(new Error('oops'));
+	const {message} = await t.throwsAsync(pipeline(failingStream, duplex, makeCollector()));


Suggested change

const {message} = await t.throwsAsync(pipeline(failingStream, duplex, makeCollector()));

await t.throwsAsync(pipeline(failingStream, duplex, makeCollector()), {message: 'oops'});

sindresorhus · 2020-06-01T18:04:05Z

should I squash the commits ?

No need. We'll squash when merging.

sindresorhus · 2020-06-01T18:06:42Z

I would like to see even more tests for this. It's a complicated feature (streams have a lot of edge-cases).

sloonz · 2020-06-17T13:05:24Z

Sorry for the hiatus.

New iteration of the branch is there.

No new test because my dumb brain can't generate any idea of what to test that isn't currently covered. Do you have any suggestion ?

sloonz · 2020-07-08T19:40:16Z

Small bump ?

sindresorhus · 2020-08-10T21:06:26Z

Would be nice to DRY up some code between the duplex method and the others.

sindresorhus · 2020-08-10T21:06:31Z

index.js

+					return error;
+				}
+			})(),
+			new Promise((resolve, reject) => { // eslint-disable-line no-unused-vars


Don't disable the rule. Just adhere to it.

sindresorhus · 2020-08-10T21:06:31Z

index.js

+	})();
+
+	(async () => {
+		const promisesResult = (await Promise.all([


Use destructuring.

sindresorhus · 2020-08-10T21:06:31Z

index.js

+			(async () => {
+				try {
+					await result;
+					return null;


Suggested change

return null;

return;

use undefined

sindresorhus · 2020-08-10T21:06:31Z

index.js

+				output.on('close', resolve);
+			})
+		]));
+		if (promisesResult[0] !== null) {


Suggested change

if (promisesResult[0] !== null) {

if (promisesResult[0] !== null) {

sindresorhus · 2020-08-10T21:06:31Z

index.js

+	} catch (error) {
+		const errorPromise = Promise.reject(makeError({
+			error,
+			stdout: undefined,


This should not be undefined.

sindresorhus · 2020-08-10T21:06:31Z

test/duplex.js

+			callback();
+		}
+	});
+	writableStream.collect = () => Buffer.concat(chunks).toString('utf-8');


Suggested change

writableStream.collect = () => Buffer.concat(chunks).toString('utf-8');

writableStream.collect = () => Buffer.concat(chunks).toString('utf8');

sindresorhus · 2020-08-10T21:06:31Z

test/duplex.js

+	t.is(message, 'oops');
+	const error = await t.throwsAsync(duplex);
+	t.is(error.isCanceled, true);
+	t.is(error.killed, true);


Suggested change

t.is(error.killed, true);

t.true(error.killed);

sindresorhus · 2020-08-10T21:07:29Z

In general for PRs:

Look over your own diff.
Try to improve the implementation.
Try to refactor and simplify.
Improve docs.

sloonz · 2020-09-23T14:50:14Z

I would like your opinion on something.

I planned to refactor in two parts, but now I wonder if the second part is needed.

In the previous implementation the result of duplexStream was both a duplex stream AND and execa object (spawned process object + promise). Now I wonder if this is a good idea, or if the result of duplexStream should not be just a stream and don't return the spawned process or a promise at all

Pros: way simpler code and interface
Cons: can't wait for the process to be killed after a failure in the pipeline (a kill signal is sent if the pipeline fails, but with just the duplex stream you have no way of waiting for the process to actually have ended) ; can't get the exit code (or really any information) of the process after a failure in the pipeline. I think the biggest con is that I see no way of unit testing "pipeline failure should kill the process".

I tend to prefer the simpler solution, but at the end it's your call :)

sloonz added 9 commits May 15, 2020

[wip] duplex-stream

9708d7c

[wip] fix lint issues

e7b9d44

[wip] fix rejection handling issues

f5b2d19

[wip] typescript types

3f53756

[wip] linter

b3e50c5

[wip] unit tests

91d8532

[wip] do not destroy when process ends

d2b8b3c

process end event may come before stdin/stdout streams flush

[wip] fix and test invalid arguments handling

Loading status checks…

0ffaeb6

[wip] test all

Loading status checks…

f0f2c5b

sindresorhus reviewed May 16, 2020

View changes

sloonz added 2 commits May 17, 2020

[wip] err -> error

Loading status checks…

6c29324

[wip] fix inconsistent behavior between node 12 & 14

Loading status checks…

28ac048

[wip] fix inconsistent behavior between windows and linux on tests

Loading status checks…

7cbc2ad

sloonz force-pushed the sloonz:duplex-stream branch 2 times, most recently from 6b273d5 to 7cbc2ad May 25, 2020

[wip] more tests and fixes

Loading status checks…

fd24f91

sloonz force-pushed the sloonz:duplex-stream branch from 53be384 to fd24f91 May 26, 2020

Update index.js

Verified

This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.

GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits

Loading status checks…

b64006f

sindresorhus requested changes Jun 1, 2020

View changes

sloonz added 2 commits Jun 16, 2020

[wip] use async instead of promise.then

75585a9

[wip] variables renames/comments changes

Loading status checks…

fc94b81

sindresorhus requested changes Aug 10, 2020

View changes

[wip] refactor part 1

Loading status checks…

264de4e

Dec	JAN	Feb
	03
2020	2021	2022

sindresorhus / execa

Duplex stream #424

Duplex stream #424

sloonz commented May 15, 2020 •

edited by sindresorhus

This comment has been minimized.

sindresorhus commented May 16, 2020

This comment has been minimized.

jschatz1 commented on `index.js` in `9708d7c` May 17, 2020

sloonz commented May 17, 2020

sloonz commented May 21, 2020

sloonz commented May 26, 2020 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

sindresorhus commented Jun 1, 2020 •

edited

sindresorhus commented Jun 1, 2020

sloonz commented Jun 17, 2020

sloonz commented Jul 8, 2020

sindresorhus commented Aug 10, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

sindresorhus commented Aug 10, 2020

sloonz commented Sep 23, 2020

	Execute a file as a transform stream
	Execute a file as a transform stream.

	write(chunk, encoding, cb) {
	write(chunk, encoding, callback) {

	const {message} = await t.throwsAsync(pipeline(failingStream, duplex, makeCollector()));
	await t.throwsAsync(pipeline(failingStream, duplex, makeCollector()), {message: 'oops'});

	if (promisesResult[0] !== null) {

	if (promisesResult[0] !== null) {

	writableStream.collect = () => Buffer.concat(chunks).toString('utf-8');
	writableStream.collect = () => Buffer.concat(chunks).toString('utf8');

sindresorhus / execa

Sponsor sindresorhus/execa

Duplex stream #424

Duplex stream #424

Conversation

sloonz commented May 15, 2020 • edited by sindresorhus

This comment has been minimized.

sindresorhus May 16, 2020 Owner

sindresorhus commented May 16, 2020

This comment has been minimized.

jschatz1 commented on index.js in 9708d7c May 17, 2020

sloonz commented May 17, 2020

sloonz commented May 21, 2020

sloonz commented May 26, 2020 • edited

This comment has been minimized.

sindresorhus Jun 1, 2020 Owner

This comment has been minimized.

sindresorhus Jun 1, 2020 Owner

This comment has been minimized.

sindresorhus Jun 1, 2020 Owner

This comment has been minimized.

sloonz Jun 17, 2020 Author

This comment has been minimized.

sindresorhus Jun 1, 2020 Owner

This comment has been minimized.

sindresorhus Jun 1, 2020 Owner

This comment has been minimized.

sindresorhus Jun 1, 2020 Owner

This comment has been minimized.

sindresorhus Jun 1, 2020 Owner

sindresorhus commented Jun 1, 2020 • edited

sindresorhus commented Jun 1, 2020

sloonz commented Jun 17, 2020

sloonz commented Jul 8, 2020

sindresorhus commented Aug 10, 2020

This comment has been minimized.

sindresorhus Aug 10, 2020 Owner

This comment has been minimized.

sindresorhus Aug 10, 2020 Owner

This comment has been minimized.

sindresorhus Aug 10, 2020 Owner

This comment has been minimized.

sindresorhus Aug 10, 2020 Owner

This comment has been minimized.

sindresorhus Aug 10, 2020 Owner

This comment has been minimized.

sindresorhus Aug 10, 2020 Owner

This comment has been minimized.

sindresorhus Aug 10, 2020 Owner

sindresorhus commented Aug 10, 2020

sloonz commented Sep 23, 2020

sloonz commented May 15, 2020 •

edited by sindresorhus

sindresorhus May 16, 2020
Owner

jschatz1 commented on `index.js` in `9708d7c` May 17, 2020

sloonz commented May 26, 2020 •

edited

sindresorhus Jun 1, 2020
Owner

sindresorhus Jun 1, 2020
Owner

sindresorhus Jun 1, 2020
Owner

sloonz Jun 17, 2020
Author

sindresorhus Jun 1, 2020
Owner

sindresorhus Jun 1, 2020
Owner

sindresorhus Jun 1, 2020
Owner

sindresorhus Jun 1, 2020
Owner

sindresorhus commented Jun 1, 2020 •

edited

sindresorhus Aug 10, 2020
Owner

sindresorhus Aug 10, 2020
Owner

sindresorhus Aug 10, 2020
Owner

sindresorhus Aug 10, 2020
Owner

sindresorhus Aug 10, 2020
Owner

sindresorhus Aug 10, 2020
Owner

sindresorhus Aug 10, 2020
Owner