Update types stream.Readable with NodeJS.ReadableStream

[ZhangYiJiang]

In TypeScript, NodeJS.ReadableStream is the interface, whereas stream.Readable is a specific implementation of NodeJS.ReadableStream. I believe this library should work with all readable streams, and so should accept the more general interface rather than just stream.Readable

[dougwilson]

Thank you! I'm not familair with TypeScript, so cannot review this change, but perhaps you can help answer some questions around it that may help me:

1. What would be an example of code that would not work with the existing types that would work with the updated types?
2. Is this change backwards compatible with existing TypeScript code out there?
3. How does NodeJS get into the typings without an import?

Thank you!

[dougwilson]

/cc @blakeembrey who contributed the original types to take a look too 👍

[blakeembrey]

SGTM, I think this makes sense. I think ReadableStream either didn't exist or had issues when I originally implemented these typings. However, it'd also be useful to understand how this broke for you.

To answer 2, it depends if people are up-to-date with @types/node, but I think this is reasonable to implement (this could be considered a fix/patch). For 3, TypeScript has a built-in concept of typeRoots and types that are global. Once you install @types/node, it would automatically work.

[ZhangYiJiang]

For 1: This would make the library compatible with the many libraries that use the ReadableStream interface instead of the concrete class - https://github.com/DefinitelyTyped/DefinitelyTyped/search?q=readablestream&unscoped_q=readablestream (a bit arbitrary, but it's a bit hard to show that that the interface is used more than the class)

For 2: It should be since the interface is "wider" than the class since the class already implements the interface, and I can't imagine any code importing from stream, a native node module, to not have @types/node

[dougwilson]

Thanks both of you 🤗 . I wanted to add a test if someone can provide number 1 prior to merge, thanks again!

[dougwilson]

Any update on providing the information above? Also, does this need to have /// <reference types="node" /> added to the typings file as well?