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

Fix ReadableStream incorrectly marked as a sync iterable #1134

Merged
merged 1 commit into from
Sep 4, 2021
Merged

Fix ReadableStream incorrectly marked as a sync iterable #1134

merged 1 commit into from
Sep 4, 2021

Conversation

MattiasBuelens
Copy link
Contributor

It looks like a regression slipped into the ReadableStream interface. In TypeScript 4.4, it incorrectly has a [Symbol.iterator] method, along with automatic forEach/entries/keys/values methods. In reality ReadableStream is an async iterable<any>, meaning it has a [Symbol.asyncIterator] and values method (both returning an async iterator).

Since we don't (yet) have a way to express this, I've changed the emitter to instead ignore async iterable declarations, so at least we don't add any non-existent methods. Alternatively, we could do this properly: tackle microsoft/TypeScript#29867 and introduce lib.dom.asynciterable.d.ts.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2021

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

Copy link
Contributor

@saschanaz saschanaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems nobody really has implemented async iterable for ReadableStream, so lgtm.

@saschanaz
Copy link
Contributor

@github-actions, lgtm again?

@github-actions github-actions bot merged commit 40991f3 into microsoft:main Sep 4, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2021

Merging because @saschanaz is a code-owner of all the changes - thanks!

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 this pull request may close these issues.

2 participants