Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReadableStream typing does not match async iterator spec #141

Closed
jacoblee93 opened this issue Jan 8, 2024 · 3 comments · Fixed by #142
Closed

ReadableStream typing does not match async iterator spec #141

jacoblee93 opened this issue Jan 8, 2024 · 3 comments · Fixed by #142

Comments

@jacoblee93
Copy link

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:

openai/openai-node#613
langchain-ai/langchainjs#3793

@MattiasBuelens
Copy link
Owner

MattiasBuelens commented Jan 10, 2024

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 openai/openai-node#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
Copy link
Author

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

@jacoblee93
Copy link
Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants