Skip to content

Add make and convenience functions for async iterators #243

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

Merged
merged 3 commits into from
Aug 29, 2024

Conversation

zth
Copy link
Collaborator

@zth zth commented Aug 26, 2024

This adds a make function and value + done convenience functions for async iterators.

cc @cometkim

@zth zth requested review from cknitt and fhammerschmidt August 26, 2024 14:55
Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

LGTM (but I have never used an AsyncIterator before)

@zth
Copy link
Collaborator Author

zth commented Aug 26, 2024

Curious about feedback from @cometkim

@cometkim
Copy link
Member

I expect the producer and consumer of an iterator to work cooperatively.

However, in this example, only the producer can decide to stop, or it must expose its internal state to the outside world. This negates most of the benefits of the abstraction and is also of little use in FFI.

This is why @JonoPrest and I extended the topic in Retreat to include break/continue and generators.

https://github.com/JonoPrest/iterator-rescript/blob/retreat-note/RETREAT_NOTE.md

@zth
Copy link
Collaborator Author

zth commented Aug 26, 2024

@cometkim sure, and when that lands that'll be a much better solution. This a little something up until the point that it does land.

@cometkim
Copy link
Member

cometkim commented Aug 26, 2024

I'd like to know the motivation and use case for adding this. I'm not sure if this would be useful without break/continue.

But it's easy to support if we add a small runtime for it.
e.g. https://github.com/JonoPrest/iterator-rescript/blob/retreat-note/src/Demo.res

@zth
Copy link
Collaborator Author

zth commented Aug 26, 2024

I'd like to know the motivation and use case for adding this. I'm not sure if this would be useful without break/continue.

But it's easy to support if we add a small runtime for it. e.g. https://github.com/JonoPrest/iterator-rescript/blob/retreat-note/src/Demo.res

There are places where you need to return an actual AsyncIterator. One example is graphql-js subscriptions.

@cometkim
Copy link
Member

cometkim commented Aug 26, 2024

Yes. The binding to iterator types and supporting a factory will be useful for FFIs. I had thought that forEach was newly added here.

I think adding a factory is ok, If the AsyncIterator.t type privides a proper forward compatibility.

@zth
Copy link
Collaborator Author

zth commented Aug 27, 2024

Yes. The binding to iterator types and supporting a factory will be useful for FFIs. I had thought that forEach was newly added here.

I think adding a factory is ok, If the AsyncIterator.t type privides a proper forward compatibility.

I guess it would, no?

@zth
Copy link
Collaborator Author

zth commented Aug 29, 2024

@cometkim in your eyes, can we merge this or are there still outstanding questions you want answers to?

@cometkim
Copy link
Member

Yes. The binding to iterator types and supporting a factory will be useful for FFIs. I had thought that forEach was newly added here.
I think adding a factory is ok, If the AsyncIterator.t type privides a proper forward compatibility.

I guess it would, no?

I think it would, or it can be easily migrated.

@cometkim
Copy link
Member

forEach doesn't seem useful, but that's not related to this PR. Right?

@zth
Copy link
Collaborator Author

zth commented Aug 29, 2024

forEach doesn't seem useful, but that's not related to this PR. Right?

Not related to this PR, no. Why is it not useful though? I use it myself in a few places.

@zth zth merged commit ddc4a08 into main Aug 29, 2024
6 checks passed
@zth zth deleted the create-async-iterators branch August 29, 2024 12:47
@cometkim
Copy link
Member

forEach doesn't seem useful, but that's not related to this PR. Right?

Not related to this PR, no. Why is it not useful though? I use it myself in a few places.

As already mentioned, if the iterable is not array-like, its usability is limited without control flow support.

Or it can be (more) useful when combined with additional utilities like take(limit).

https://github.com/tc39/proposal-iterator-helpers/?tab=readme-ov-file#takelimit

@zth
Copy link
Collaborator Author

zth commented Aug 29, 2024

Right, got it!

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.

4 participants