Add Symbol.asyncIterator #4967
Add Symbol.asyncIterator #4967
Conversation
| @@ -251,6 +252,37 @@ class JSSymbolTest { | |||
| assertArrayEquals(Array(532), content.result()) | |||
| } | |||
|
|
|||
| @Test def nativeAsyncIterable(): Unit = { | |||
There was a problem hiding this comment.
I don't currently understand the full implications for why the existing equivalent test for Symbol.iterator (i.e. nativeIterable) is implemented the way it is. In particular, it's not clear to me why:
- the
Array.from(iterable)constructor is picked out for testing against both the class and trait facades - the
iterateIterable-related checks are only performed for the facade trait
Anyway, here's my crack at an equivalent test for Symbol.asyncIterator.
I did take a look at implementing Array.fromAsync(asyncIterable). MDN suggests this feature has support across most browsers already, however its TC39 proposal is still progressing.
There was a problem hiding this comment.
The purpose of nativeIterable is to make sure that our mock implementation of the Symbol.iterator protocol corresponds to the spec. We do this by invoking a library function known to be a client of that protocol. That gives us confidence that the sjsdefinedIterable test is actually meaningful. So in a way, nativeIterable tests the other test.
As is, nativeAsyncIterable does not seem to fulfill the same purpose. If there is no Stage-4 library function that we can use as client of the API, we can turn it around. We can use a native provider (probably a js.Array), pass it through our client code iterateAsyncIterable, and check the result. This way we test that iterateAsyncIterable is a correct client of the API. That is also enough to test that sjsdefinedAsyncIterable is testing what it is designed to test.
There was a problem hiding this comment.
Thanks very much for the detailed explanation. I understand this much better now.
I haven't been able to find any viable alternative clients, and unfortunately, MDN suggests we've a problem with going down the native provider approach too.
There is no object in the core JavaScript language that is async iterable.
It seems to me like we should park this PR until there's a suitable way to validate the protocol behaviour. What do you think?
There was a problem hiding this comment.
No, the inability to do a double-test shouldn't hold this PR, IMO. If we don't have any native way to exercise the protocol, let's keep only our user level way of testing it. At least that tests that the symbol actually exists, which at the end of the day is what users can rely on.
There was a problem hiding this comment.
Cool, I'm happy with that. I've removed the nativeAsyncIterable test now and have left the sjsdefinedAsyncIterable test in place.
There was a problem hiding this comment.
Sorry, I'm late to this. Doesn't an async for loop for await (... consume the async iterator protocol natively?
|
@sjrd were you planning on reviewing this? IMO it's more in your area of expertise. |
|
Ouch yes, I completely missed this one. I'll review tomorrow. |
There was a problem hiding this comment.
Hey @DanielMoss. I'm terribly sorry for the long wait. If you are still interested in seeing this through, here are some comments.
test-suite/js/src/test/scala/org/scalajs/testsuite/jsinterop/JSSymbolTest.scala
Outdated
Show resolved
Hide resolved
test-suite/js/src/test/scala/org/scalajs/testsuite/jsinterop/JSSymbolTest.scala
Outdated
Show resolved
Hide resolved
| @@ -251,6 +252,37 @@ class JSSymbolTest { | |||
| assertArrayEquals(Array(532), content.result()) | |||
| } | |||
|
|
|||
| @Test def nativeAsyncIterable(): Unit = { | |||
There was a problem hiding this comment.
The purpose of nativeIterable is to make sure that our mock implementation of the Symbol.iterator protocol corresponds to the spec. We do this by invoking a library function known to be a client of that protocol. That gives us confidence that the sjsdefinedIterable test is actually meaningful. So in a way, nativeIterable tests the other test.
As is, nativeAsyncIterable does not seem to fulfill the same purpose. If there is no Stage-4 library function that we can use as client of the API, we can turn it around. We can use a native provider (probably a js.Array), pass it through our client code iterateAsyncIterable, and check the result. This way we test that iterateAsyncIterable is a correct client of the API. That is also enough to test that sjsdefinedAsyncIterable is testing what it is designed to test.
|
Cheers guys. Appreciate the feedback and no problem on the delay. I'll take a look at sorting out the unit test shortly. |
|
The code changes look good now. There are only "administrative" details to get right for the commit history and versioning.
After that it's good to go. Thank you for your patience. |
6a7dffa to
4a1e347
Compare
Hi!
This symbol returns an object conforming to the async iterator protocol. This protocol is effectively the same as the regular iterator protocol, except the methods on the iterator return values wrapped in promises. Any and all feedback is welcome.
This is a follow-up of my message in the Scala.js discord, where I mentioned I was interested in adding facades for working with the OPFS to scala-js-dom. In particular,
Symbol.asyncIteratoris required for a complete implementation of theFileSystemDirectoryHandlefacade. Assuming I'm going about this the right way, my next step after this PR would be to take a look at adding anAsyncIteratorfacade to this repo.Doc links: