got.stream is not pipable on error

[dangkyokhoang]

Describe the bug

• Node.js version: 15.11.0
• got version: 11.8.2
• OS & version: Windows

Actual behavior

response.rawBody <Buffer ...>

On error, got.stream response is not pipable. The response body is under response.rawBody which is supposed to be "only accessible when using Promise API" (https://github.com/sindresorhus/got/blob/main/documentation/3-streams.md#rawbody). I would assume there's no data event as the response body has already been consumed?

Expected behavior

on data <Buffer ...>

Code to reproduce

const got = require('got')

const url = 'https://run.mocky.io/v3/99fc1661-4a34-4334-a123-ae9c02b33db4'

got.stream(url)
  .on('error', error => {
    const { response } = error

    console.log('response.rawBody', response.rawBody)
  })
  .on('data', chunk => {
    console.log('on data', chunk)
  })

Checklist

• I have read the documentation.
• I have tried my code with the latest version of Node.js and Got.

[szmarczak]

I would assume there's no data event as the response body has already been consumed?

More or like. This is rather a design issue. HTTPError will be thrown so in order to see the body you can access it via error.response.rawBody. Although one can expect data events to be emitted - and this is a valid argument. If you wanted to know the status code prior to reading (e.g. cancel the stream), you'd have to do this:

stream.on('response', response => {
	if (got.isResponseOk(response)) { // `response.statusCode`, `response.headers` can be accessed as well
		// process the stream as usual
	} else {
		stream.destroy(new Error('stream canceled'));
	}
});

stream.pipe(writable);

There's a need for an additional event. This could be solved by making got.stream return a Promise. I like that approach, because it normalizes the return type and is more convenient than attaching another event. undici does this as well. So it could look like:

const stream = await got.stream('https://example.com', {signal: controller.signal});
if (!stream.ok) { // `stream.statusCode`, `stream.headers` could be accessed as well
	controller.abort();
	return;
}

stream.pipe(writable);

/cc @sindresorhus

[sindresorhus]

I agree with the suggested change, but we just did a major version and we probably won't do another one for quite some time.

How about instead of doing a breaking change to got.stream, we instead embrace Web Streams and add a got.webStream() method that returns a web stream instead? Maybe there are other API improvements we can do to that method while we're at it. We could mark the API as experimental and tell users to use a fixed dependency range. This would give us a chance to experiment with the API.