replace raw mapAsyncIterable with Repeater based approach

[yaacovCR]

The basic idea behind this PR is that graphql-js should not export something with type signature of an async generator that sometimes does not behave like an async generator.

Repeaters — thanks to the amazing work of @brainkim — can exhibit behavior that matches exactly to that of async generators.

In particular:

1. Calls to next(), return(), and throw() will not interfere with each other; they will be executed sequentially.
2. The promises returned by calls to Calls to next(), return(), and throw() will all settle in call order.

Additional motivation: graphql-executor builds on Repeaters to create a Publisher abstraction that allows the executor to push results to the generator as they are ready, without the need for any complicated promise racing.

[netlify[bot]]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	376d192adb063630762a1f372a36d2aca9c7de7f
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/630531ddad63110008a1b8ea
😎 Deploy Preview	https://deploy-preview-3694--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[github-actions[bot]]

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

• @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
• @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

[yaacovCR]

Some of the changes in the tests for MapAsyncIterable are because the current graphql-js deviates from the spec with regard to .return(). An AsyncGenerator should yield a final payload with its value set as the argument to return. If the original AsyncIterable has a .return() method, it should be called to clean up, but the return value should essentially be ignored.

See: https://github.com/repeaterjs/repeater/blob/219a0c8faf2c2768d234ecfe8dd21d455a4a98fe/packages/repeater/src/repeater.ts#L556

However, some of the changes are because I have not yet gotten my Repeater-based map transducer to properly handle a "recovered" call to .return(), i.e. payloads yielded from the finally block of an underlying AsyncGenerator.

Note this comment: https://github.com/repeaterjs/repeater/issues/72#issuecomment-963426268

Returns are non-recoverable; you can theoretically use a try/finally to yield more values, but no one should be doing that.

Unfortunately, despite the fact that no one should be doing that, perhaps they are, and the current mapAsyncIterable supports this behavior. So our Repeater-based transducer (or Repeaters themselves) apparently need some fine-tuning?

Investigation continues. @brainkim @IvanGoncharov et al, all thoughts/comments are welcome.

[yaacovCR]

Note that the Repeater-based map transducer in this PR is basically from https://github.com/repeaterjs/repeater/issues/48#issuecomment-569134039

With the additional ability to forward .throw() calls to the underlying iterable.

[yaacovCR]

Perhaps of interest: https://github.com/microsoft/TypeScript/issues/45400

[yaacovCR]

I realized that the behavior of the current implementation of mapAsyncIterable that I called not-to-spec, i.e. that it forwards the return value of the source iterable rather than passing along the value passed to return, is kind of debatable. The purpose of the function is to map the source iterable, and so if the source iterable deviates from the spec, which by the spec "is not enforced," it's not clear that the mapper should hide this from the caller.

I have therefore reverted my test changes in this regard.

I have also completed the changes within the repeater-based map iterable to map iterables that continue to yield values even after their return method is called.

This PR is now ready for review.

In summary, with no significant change to the mapAsyncIterable tests, the repeater based approach shows how the current implementation gives you an object that has the same API as a generator, but is not actually a generator, in that calls to then, return, and throw calls are not evaluated sequentially, and the promises they return are not always resolved sequentially. Using Repeaters, we return an object that matches both the signature and the behavior of a generator.

I have also updated the PR description above to add an additional motivating point with regard to incremental delivery.

[yaacovCR]

Wow -- how silly of me!

In changing Repeaters to behave exactly-exactly-exactly like AsyncGenerators, I had to give up on concurrent returns.

Meaning, if a for await loop is iterating through the repeater, and you call the repeater's .return() from elsewhere in your code, the pending .then() will always return first prior to the .return(), so:

(A) you run into the secure stop problem that maybe we should still avoid (B) at that point, at least for the map and flatten, transducers, you could just use an actual generator rather than a fancy repeater. (It's still worth it to use repeaters to manage the queue for incremental delivery, however, because writing an actual generator is difficult because you can't enclose the yield keyword and pass it into the executor, like you can with push/stop with repeaters.

But we should really all get on the same page as to whether we:

1. want concurrent returns: AsyncGenerators don't have them, our current "TS-signature AsyncGenerator-like" object does have them, and Repeaters do have them.

2. want concurrent throws: AsyncGenerators don't have them, our current "TS-signature AsyncGenerator-like" object does have them, and Repeaters do NOT have them.

3. want throws at all -- consumers of the "generators" returned by execute should never really use .throw(). They should indicate that they are done with the generator by calling .return(), and the generator should never really care why. Using .throw() (or .return(...) with an argument) implies a shared contract with how the generator is implemented, and that's just not happening in our use case.

4. want to make sure that all calls to .next(), etc, settle in call order: Async Generators always does, our current "TS-signature AsyncGenerator-like" object does not necessarily, and Repeaters always do.

5. want to align with https://tc39.es/proposal-iterator-helpers/ -- hat tip to @IvanGoncharov for pointing this out --. As far as I can tell, what we have with that proposal is an object that is an iterator with an argument-less return method, not technically a generator, but that behaves like a generator in that (a) calls to .then() and .return() are NOT concurrent (b) results of those promises settle in call order. We can easily change our repeater implementation to mimic this: we simply take what I have put together and drop the .then() support, drop support for arguments to .return(),and keep my changes that removed the support for concurrent calls to .next() and .return().

Whew! That's a lot to think about.

Does the spec have anything to say about all this? Not that I could find. Cancellation is discussed, but it doesn't seem to specify what should happen to a previously requested payload if a concurrent unsubscribe is triggered.

This sounds like a good discussion topic for the next js-wg meeting!

[yaacovCR]

I'm going to close this stale PR, at least for now. I think the way forward should be to follow along with https://tc39.es/proposal-iterator-helpers/ and that iterating prematurely would add more unhelpful noise. As far as I know, we have not had add actual library users raise issue with the potentially misleading signature we are using.