MattiasBuelens / web-streams-polyfill

Web Streams, based on the WHATWG spec reference implementation
MIT License
289 stars 29 forks source link

ReadableStream typing does not match async iterator spec #141

Closed jacoblee93 closed 9 months ago

jacoblee93 commented 9 months ago

The globally declared Symbol.asyncIterator property here should be a method, not a property.

https://github.com/MattiasBuelens/web-streams-polyfill/blob/master/dist/types/ts3.6/polyfill.d.ts#L26

vs.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/AsyncIterator/@@asyncIterator

This causes subtle downstream typing bugs like this:

node_modules/@langchain/core/dist/utils/stream.d.ts:1:18 - error TS2320: Interface 'IterableReadableStreamInterface<T>' cannot simultaneously extend types 'ReadableStream<T>' and 'AsyncGenerator<T, any, unknown>'.
  Named property '[Symbol.asyncIterator]' of types 'ReadableStream<T>' and 'AsyncGenerator<T, any, unknown>' are not identical.

See:

https://github.com/openai/openai-node/issues/613 https://github.com/langchain-ai/langchainjs/issues/3793

MattiasBuelens commented 9 months ago

Thanks for the report!

The reason I defined it as a property rather than a method was because the spec demands that ReadableStream.prototype[Symbol.asyncIterator] === ReadableStream.prototype.values, so I have to do some trickery to make that work. Unfortunately, I didn't consider that this would be visible in the public type definitions... Sorry about that! I'm working on a fix.

I see that you've decided to switch to Node's native ReadableStream implementation in https://github.com/openai/openai-node/pull/614. That's awesome, I'm happy to see streams get more native support! 😁

By the way, I noticed this type definition in your code:

export interface IterableReadableStreamInterface<T>
  extends ReadableStream<T>,
    AsyncGenerator<T> {}

That should really be:

export interface IterableReadableStreamInterface<T>
  extends ReadableStream<T>,
    AsyncIterable<T> {}

since a ReadableStream is not really an async generator. 😉

jacoblee93 commented 9 months ago

Ah got it - thanks for the context and heads up around that typing!

jacoblee93 commented 9 months ago

Thank you!