Fix AsyncIterator.prototype.flatMap to flatMap async iterables

[Jamesernator]

Currently the spec seems to flatMap only sync iterables from within async iterables, this is presumably a bug as it Awaits the IteratorNext() anyway.

This also fixes that and an unrelated typo referring to a non-existent variable nextNext.

[michaelficarra]

Thanks, LGTM.

[Jamesernator]

Actually I just realized this isn't quite correct, because it passes the (potentially) sync method to GetIterator it won't get correctly wrapped in AsyncFromSyncIterator as it skips that step if the method is already provided.

[Jamesernator]

Perhaps the best way would be to change GetIterator to optionally return undefined if it doesn't find the given method e.g.:

// New parameter supresses error if iterator
// method does not exist and instead returns undefind
let _innerIterator_ be ? GetIterator(_mapped_, ~async~, *undefined*, *false*).
If _innerIterator_ is *undefined*, then
  Perform ? Yield(_mapped_).
Else,
  // ... Iterate over 

[ljharb]

I think the spec usually uses GetMethod here rather than GetIterator for that?

[Jamesernator]

I think the spec usually uses GetMethod here rather than GetIterator for that?

So the issue with my PR is that calling GetIterator(_mapped_, ~async~, _usingIterator_) means that if _usingIterator_ happened to be a sync method it doesn't get wrapped in an AsyncFromSyncIterator.

The other option is to do something like:

1. Let _isSyncIterator_ be *false*.
1. Let _usingIterator_ be ? Get(_mapped_, @@asyncIterator).
1. If _usingIterator_ is *undefined*, then
  1. Set _usingIterator_ to ? Get(_mapped_, @@iterator).
  1. If _usingIterator_ is not *undefined*, then
    1. Set _isSyncIterator_ to *true*.
1. If _usingIterator_ is *undefined*,
  1. Perform ? Yield(_mapped_).
1. Else,
  1. Let _innerIterator_ be *undefined*.
  1. If _isSyncIterator_ is *true*, then
    1. Let _syncIterator_ be ? GetIterator(_mapped_, ~sync~, _usingIterator_).
    1. Set _innerIterator_ to ? CreateAsyncFromSyncIterator(_syncIterator_).
  1. Else,
    1. Set _innerIterator_ to ? CreateIterator(_mapped_, ~async~, _usingIterator_).
  1. Let _innerAlive_ be *true*.
  // ... Proceed with rest of steps of flat-mapping

(NOTE: The only reason for Get(_mapped_, @@asyncIterator) over GetMethod(_mapped_, @@asyncIterator) is because that's what the current proposed spec does, I agree GetMethod is probably preferable as this it what GetIterator does internally).

This seems kinda redundant though as it's basically just the implementation of GetIterator (check async, then check sync, if sync then wrap it, return iterator) except without the TypeError if neither method exists.

[devsnek]

fwiw this is why i hadn't added the async-specific code yet :P

I think adding a param to GetIterator is okay, although it should probably be more like ~nothrow~, not *false*. We could also make GetIterator return undefined, and add a GetIteratorOrThrow which throws if its undefined.

[Jamesernator]

I think adding a param to GetIterator is okay, although it should probably be more like nothrow, not false. We could also make GetIterator return undefined, and add a GetIteratorOrThrow which throws if its undefined.

I agree, I'm not super familiar with the markup so I wasn't sure what to put.

I'll make another PR that replaces GetIterator with GetIterator + GetIteratorOrThrow.

[bakkot]

@Jamesernator I don't think you need to do the AsyncFromSyncIterator wrapping, strictly speaking, because its results are all passed through the Await macro (which wraps non-promises in promises anyway). I'm not sure why AsyncFromSyncIterator exists, actually.