nodejs / readable-stream

Node-core streams for userland
https://nodejs.org/api/stream.html
Other
1.03k stars 225 forks source link

Array-like methods on Readable #524

Closed Touffy closed 1 year ago

Touffy commented 1 year ago

Hi there. I am trying to add types for the Array-like methods on Readable and running into issues with readable-stream and a few of its dependents. They make the assumption that Readables created by readable-stream should be assignable to Node's own Readable type, which is not true when only Node implements those methods and not readable-stream.

Besides typing inconsistencies, the main argument for implementing the methods is that they are extremely useful and, in some cases (like concurrent map/filter) tricky to implement properly. Although I can foresee some pain when the async iterator helpers proposal at at TC39 moves forward since Readables are also asyncIterable. Hope you guys are keeping a close eye on that one and the proposal eventually lines up with what's already in Node !

For the moment, I've had to cripple a few TypeScript tests (including in @types/readable-stream).

Complete list of missing methods :

benjamingr commented 1 year ago

ts. They make the assumption that Readables created by readable-stream should be assignable to Node's own Readable type, which is not true when only Node implements those methods and not readable-stream.

Can you clarify on that? You mean interop between Node streams and readable-stream "userland" Readable streams? nvm just saw what repo I'm in.

I guess that's also a TypeScrpit types fix? What in this package doesn't fit the native Readable?

proposal at at TC39 moves forward since Readables are also asyncIterable. Hope you guys are keeping a close eye on that one and the proposal eventually lines up with what's already in Node !

Everything should (ideally) align and change according to that proposal :)

Touffy commented 1 year ago

The TypeScript mismatch is just the part that annoys me specifically and had me disable a few of your TS assertions in @types/readable-stream. I guess it may not be totally clear in my issue, but I posted here to suggest that you actually implement the methods. That way, users of readable-stream get those cool methods, and the type of your Readable will naturally match Node's so we can restore the tests over in DefinitelyTyped.

benjamingr commented 1 year ago

We... do?

import { Readable } from 'readable-stream';
const result = await Readable.from([1,2,3,4]).map(x => x * 2).reduce((p, c) => p + c);
console.log(result); // 20
Touffy commented 1 year ago

That shouldn't work without an await. But this is good news. All that remains missing is the types, then. Shall I close this issue or rephrase it to just ask for an update to @types/readable-stream ?

Touffy commented 1 year ago

Actually, I can add the types myself. After all, it should be mostly a copy-paste of my definitions for Node.