Don't use instanceof stream.Readable, it's an interface

[Mstrodl]

stream.Readable doesn't need to be extended from, it's just an interface that readable streams implement. I replaced body instanceof stream.Readable with is.nodeStream() because that's the check that was used in other places of the code.

Specifically, I was trying to use archiver with chrome-webstore-upload which recently started using got (in version 4.0) which broke my publish CI script because of this. Archiver doesn't actually extend stream.Readable, it only implements it, which caused the issue.

Checklist

• I have read the documentation.
• I have included a pull request description of my changes.
• I have included some tests.
• If it's a new feature, I have included documentation updates in both the README and the types.

[szmarczak]

it's an interface

It's not and I can prove you wrong: require('stream').Readable.constructor

I replaced body instanceof stream.Readable with is.nodeStream() because that's the check that was used in other places of the code.

Wow. Because it is used in other places doesn't mean it's correct [for this one].

I was trying to use archiver

This is not a Got issue. archiver uses legacy util.inherits. From the Node.js docs:

Usage of util.inherits() is discouraged. Please use the ES6 class and extends keywords to get language level inheritance support. Also note that the two styles are semantically incompatible.

Please raise an issue in the archiver repo instead.

[Mstrodl]

@szmarczak

It's not and I can prove you wrong: require('stream').Readable.constructor

Right, JS doesn't have interfaces, but, from the very first line of the Node.js docs:

"A stream is an abstract interface for working with streaming data in Node.js. The stream module provides an API for implementing the stream interface."

In other words, extending stream.Readable is just an easier way to implement a readable stream. There is a reason modules like is-stream work the way they do.

Wow. Because it is used in other places doesn't mean it's correct [for this one].

Yet in this case it's pretty clearly causing issues with legacy util.inherits and anything that implements a stream.Readable rather than simply extending it.

This is not a Got issue. archiver uses legacy util.inherits. From the Node.js docs:

util.inherits aside, readable streams are not strictly required to extend from stream.Readable and can instead simply implement stream.Readable's properties and, moreover, this could potentially cause issues with different versions of polyfills for example.

[Mstrodl]

By the way, your tone was kind of unnecessarily terse and rude there. I understand that you might have to deal with a lot of inexperienced people in issues, but you kind of come off as condescending. :(

[szmarczak]

your tone was kind of unnecessarily terse

Many people come to Got and report issues related to other projects because the integration simply does not work. It doesn't mean that it is actually Got's fault. In most cases it's either Node.js' or the other project's. I have proposed a solution, so the issue is solved on Got's side. Here's a deeper explaination.

and rude there.

I haven't offended you in any way. I haven't said a thing you've done incorrectly. I just stated the facts. If that's how you feel, I'm sorry.

I understand that you might have to deal with a lot of inexperienced people in issues

That is NOT the point. Even very experienced people open Got issues just to report incompatibility with other projects.

but you kind of come off as condescending

Well, it may seem so (especially when an issue is closed almost right away) but that's definitely not true. See:

I have proposed a solution, so the issue is solved on Got's side.

Exactly. archiver already targets Node.js 10 so they can switch to extends without breaking anything. I kindly suggested to open an archiver issue instead.

In other words, extending stream.Readable is just an easier way to implement a readable stream.

No, it means exactly what it says. An abstract interface is just a bare look. The stream module is a native one and all modern applications expect to get either a Readable or a Writable instance. Even Node.js fails to do this correctly: sindresorhus/is-stream#8 (see https://nodejs.org/api/stream.html#stream_implementing_a_writable_stream for how to properly implement a Writable)

There is a reason modules like is-stream work the way they do.

Yep, there is. Backwards compatibility. This is the current implementation from March 2017:

isStream.readable = stream =>
	isStream(stream) &&
	stream.readable !== false &&
	typeof stream._read === 'function' &&
	typeof stream._readableState === 'object';

And this is the one from January 2015:

isStream.readable = function (stream) {
	return isStream(stream) && typeof stream._read == 'function' && typeof stream._readableState == 'object';
};

It hasn't changed much. But it's 2020, not 2015 nor 2017.

Yet in this case it's pretty clearly causing issues with legacy util.inherits

While is.nodeStream(...) works, it may fail and accept non-streams. In its pure form it just is:

> require('@sindresorhus/is').nodeStream.toString()
'(value) => is.object(value) && is.function_(value.pipe) && !is.observable(value)'

Which you can easily mimic this behavior.

and anything that implements a stream.Readable rather than simply extending it.

util.inherits aside, readable streams are not strictly required to extend from stream.Readable and can instead simply implement stream.Readable's properties and

I wouldn't expect this to not cause issues. Node.js docs recommend this:

The stream.Readable class is extended to implement a Readable stream.

And as I've already mentioned it also discourages util.inherits.

[Mstrodl]

@szmarczak The fact of the matter is, there are legitimate cases where a stream might not want to extend Readable, and got should be able to handle it. I'm not sure what issues arise from being more lax on stream support in order to support these cases. I would be more than happy to revise this in order to check more properties than just pipe, but I really can't say that I foresee anyone accidentally passing an object that just happens to implement all of Readable to body.

From the cursory look I took the other night at how archiver works, changing it to ES6 syntax to use extends would be a nontrivial undertaking for a project that, from what I can tell, doesn't see much active development mostly due to the largely unchanging nature of the zip spec.

[szmarczak]

there are legitimate cases where a stream might not want to extend Readable, and got should be able to handle it.

Per Node.js docs there are no such legitimate cases.

I'm not sure what issues arise from being more lax on stream support in order to support these cases.

The current implementation is totally valid. No need to loose the logic.

changing it to ES6 syntax to use extends would be a nontrivial undertaking for a project

I strongly disagree. It's 2020 and the standards aren't the same as in 2015.

doesn't see much active development mostly due to the largely unchanging nature of the zip spec.

The zip specification is one factor. There are plenty others that always will require some work to do.

This discussion doesn't seem to have an end, although I have already proposed a solution. Locking.