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

[RFC] Improve core types for (Async) Iterable/Iterator/Generator #3990

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

leebyron
Copy link
Contributor

  • Changes @@iterator method to return more specific type in Generators.
  • An $Iterator is an $Iterable
  • Add optional return and throw methods to $Iterator (http://www.ecma-international.org/ecma-262/7.0/#table-54)
  • A $Generator is an $Iterator
  • Fix mistaken function name to $asyncIterate to match style of $iterate.

* Changes `@@iterator` method to return more specific type in Generators.
* An `$Iterator` is an `$Iterable`
* Add optional `return` and `throw` methods to `$Iterator` (http://www.ecma-international.org/ecma-262/7.0/#table-54)
* A `$Generator` is an `$Iterator`
* Fix mistaken function name to `$asyncIterate` to match style of `$iterate`.
@nmote nmote added the Library definitions Issues or pull requests about core library definitions label Jan 3, 2019
@goodmind
Copy link
Contributor

@nmote do you think this is ok to merge?

@@iterator(): $Iterator<Yield,Return,Next>;
next(value?: Next): IteratorResult<Yield,Return>;
+return?: <R>(value: R) => IteratorResult<Yield,R|Return>;

Choose a reason for hiding this comment

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

A return definition would be great. :)

I'd think it should just be () => IteratorResult<Yield, Return> (same for $AsyncIterator below) as long as that doesn't break passing a generator where an iterator is expected.

If you're hand-writing a return method to handle abrupt completions, you normally wouldn't accept any value because none are passed on IteratorClose. See http://www.ecma-international.org/ecma-262/7.0/#sec-iteratorclose

interface $Iterable<+Yield,+Return,-Next> {
@@iterator(): $Iterator<Yield,Return,Next>;
}
type Iterable<+T> = $Iterable<T,void,void>;

interface Generator<+Yield,+Return,-Next> {
interface $Iterator<+Yield,+Return,-Next> extends $Iterable<Yield,Return,Next> {
@@iterator(): $Iterator<Yield,Return,Next>;

Choose a reason for hiding this comment

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

I wish Flow didn't require an @@iterator method on Iterators, even though they typically have one that returns this by convention. (But I'm not sure how to use this without hacks in any case: #1163.)

Copy link
Contributor

Choose a reason for hiding this comment

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

#6075 has some relevant discussion

@goodmind
Copy link
Contributor

@nmote anything preventing merging this?

@Beanow
Copy link
Contributor

Beanow commented Mar 10, 2020

Happy comment anniversary @goodmind!
goodmind commented on Mar 10, 2019

I'm also finding lots of issues with async generators, but don't have the OCaml chops to help here.
Would love to see improvements like these 👍

@jedwards1211
Copy link
Contributor

TIL that there's a convention for AsyncIterator.return being able to accept a promise:

If the argument value is used in the typical manner, then if it is a rejected promise, a promise rejected with the same reason should be returned; if it is a fulfilled promise, then its fulfillment value should be used as the value property of the returned promise's IteratorResult object fulfillment value. However, these requirements are also not enforced.

The TS type for AsyncIterator.return, consequently, is

return?(value?: TReturn | PromiseLike<TReturn>): Promise<IteratorResult<T, TReturn>>;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Library definitions Issues or pull requests about core library definitions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants