create separate pubsub.asyncIterable method and deprecate pubsub.asyncIterator

[jedwards1211]

As @jquense wisely helped me realize in #143, pubsub.asyncIterator is unsafe and could leave dangling event listeners when consumed by 3rd-party code.

In general the documentation mistakenly states that GraphQL subscribe resolvers must return an AsyncIterator; rather, they should return an AsyncIterable, and event listener registration/anything else necessitating cleanup should only be performed when that AsyncIterable's Symbol.asyncIterator method is actually called, since for await and probably most 3rd-party code is only guaranteed to return the iterator if it ends up starting iteration (i.e. calling Symbol.asyncIterator).

As a solution, I've created a new pubsub.asyncIterable method that we should advise people to use. I also made eventEmitterToAsyncIterator print a deprecation warning when it's used like an AsyncIterable (which will happen with the current recommended usage of pubsub.asyncIterator).

The withFilter method still needs to be redesigned to operate on AsyncIterables instead of AsyncIterators.

[jedwards1211]

I have refactored the withFilter method to operate on AsyncIterables rather than AsyncIterators.

[jedwards1211]

@jonbudd actually I don't think this is a work in progress anymore...

[jedwards1211]

Although I think asyncIterable is a better API design, I was wrong that it can be totally safe.

[taion]

so IMO async iterators still give you quite a nice way to manipulate these things

i think the best way to go is to just have some explicit closeability concept; see what we did in https://github.com/4Catalyzer/graphql-subscription-server/pull/20/files

[grantwwu]

@taion But who calls that? Don't we depend on https://github.com/apollographql/subscriptions-transport-ws to call return right now? Would we need to modify that?

[taion]

the approach we use is:

1. subscribers return an async iterable and a close callback
2. we keep track of the close callbacks to call associated with each client subscription
3. we call all those close methods when we tear down the client subscription

so we just don't use return at all

[grantwwu]

Okay, so that would require modifying the transport...

[hwillson]

Thanks @jedwards1211 and sorry we didn't get to this sooner. A lot has changed with the codebase since this was opened, and some of these changes are already coming in 3.0. I'll close this off but please open any new PR's (ideally based off of the release-3.0 branch) that you would like to see added. @brainkim is helping maintain this project now as well, so the great conversation you both had in tc39/proposal-async-iteration#126 can continue in this repo.

Side note @brainkim - karma:

By the way, two issues/antipatterns I see from lurking in the GraphQL community related to lazy async iterators, but which I just haven’t really commented on because it might seem too self-promotional and I don’t really use GraphQL ...

😂