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(ext/web): ReadableStream.from() allows Iterable instead of IterableIterator #23903

Merged
merged 7 commits into from
May 27, 2024

Conversation

Milly
Copy link
Contributor

@Milly Milly commented May 20, 2024

createAsyncFromSyncIterator(x) which is used in ReadableStream.from() expects x as Iterable but, previous implements specify Iterator or IterableIterator. If it was IterableIterator, it would work, but if it was Iterator, an exception will occur.

Tests have been merged into WPT.
web-platform-tests/wpt#46365

`createAsyncFromSyncIterator(x)` expects `x` as `Iterable` but, previous
implements specify `Iterator` or `IterableIterator`. If it was
`IterableIterator`, it would work, but if it was `Iterator`, an
exception will occur.

Tests have been merged into WPT.
web-platform-tests/wpt#46365
@CLAassistant
Copy link

CLAassistant commented May 20, 2024

CLA assistant check
All committers have signed the CLA.

@bartlomieju
Copy link
Member

CC @iuioiua

@bartlomieju bartlomieju added this to the 1.44 milestone May 22, 2024
@iuioiua iuioiua changed the title fix(ext/web): ReadableStream.from() allows Iterable instead IterableIterator fix(ext/web): ReadableStream.from() allows Iterable instead of IterableIterator May 23, 2024
@iuioiua
Copy link
Collaborator

iuioiua commented May 23, 2024

I'll update this PR once #23910 is merged.

@iuioiua
Copy link
Collaborator

iuioiua commented May 23, 2024

This change fixes the following (WPT) tests:

/streams/readable-streams/from.any.html - ReadableStream.from accepts a sync iterable of values
/streams/readable-streams/from.any.html - ReadableStream.from accepts a sync iterable of promises

However, it also causes the following (WPT) tests to fail:

/streams/readable-streams/from.any.html - ReadableStream.from throws on invalid iterables; specifically an object with a non-callable @@iterator method
/streams/readable-streams/from.any.html - ReadableStream.from re-throws errors from calling the @@iterator method
/streams/readable-streams/from.any.html - ReadableStream.from ignores a null @@asyncIterator

The 3 failures all show the same error:

assert_throws_js: from() should throw a TypeError function "() => ReadableStream.from(iterable)" did not throw...

@Milly
Copy link
Contributor Author

Milly commented May 24, 2024

Currently, async generator + yield * is used to create an async iterator from a sync iterable.
That is because it implements TC39 GetIterator.

However, the test fails because yield * does not throw an error even if an invalid Iterable is given.

Currently, the getIterator() function is only used to implement ReadableStream.from().
So instead of reproducing TC39 GetIterator (creating an async iterator from a sync iterator), I think it would be better to simply create a function that returns an async iterator or a sync iterator. In that case, the caller of the function must use try-catch and await appropriately for using iterator.

@iuioiua iuioiua requested a review from crowlKats May 24, 2024 07:49
@iuioiua
Copy link
Collaborator

iuioiua commented May 24, 2024

Cool. There's been an improvement. The current failures give the same error:

assert_object_equals: first read should be correct expected property "0" missing

@Milly
Copy link
Contributor Author

Milly commented May 24, 2024

In that case, the caller of the function must use try-catch and await appropriately for using iterator.

I made a mistake by not keeping up with what I said.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thank you both @Milly and @iuioiua

@bartlomieju bartlomieju enabled auto-merge (squash) May 27, 2024 20:49
@bartlomieju bartlomieju merged commit 35e5159 into denoland:main May 27, 2024
17 checks passed
@Milly Milly deleted the fix-readablestream-from branch May 27, 2024 22:37
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.

None yet

4 participants