feat: replace iterall with native Symbol.asyncIterator + fix return types

[n1ru4l]

BREAKING fixes the type annotation of the abstract class PubSubEngine. According to the TypeScript type-defintion a PubSubAsyncIterator instance is actually an AsyncIterableIterator instead of an AsyncIterator. The typing of PubSubAsyncIterator is way more convenient as it allows iterating over it with the for await (const foo of iterator) { doSth() } syntax, which is super handy for filtering or mapping (See https://gist.github.com/n1ru4l/127178705cc0942cad0e45d425e2eb63 for some example operators).

This should be able to get merged once Node.js 10 runs out of LTS. See https://nodejs.org/en/about/releases/
Technically this project already dropped Node.js 10 support when removing the Node.js 10 CI build.

Closes #192

[robhogan]

Since this is a clean slate, would you consider changing the input types for asyncIterableIterator to accept string | readonly string[] (instead of a mutable string[]), as in #234? Changing the existing API would be another breaking change, unfortunately.

[n1ru4l]

@rh389 Yours could be merged afterwards (if any is ever merged 😅). Doing more stuff in one pr might decrease the chance of it getting merged, so I would like to keep it like that for now 😇

[robhogan]

Yeah, sorry I don't mean to hijack - just that this PR introduces asyncIterableIterator, so changing it to readonly after this is merged would require a second breaking change. Hopefully the maintainers will see this note and we can leave it up to them to change if they want to.

[n1ru4l]

@glasser I noticed you recently pushed to this repository. Is this repository actively maintained, should new maintainers, or should a deprecation notice be added? A lot of pull requests are still undressed, including this one...

[glasser]

It is not particularly maintained. It is a dependency of Apollo Server 2, for not particularly good reasons: AS does not actually use any code from it directly, but it just re-exports all of its exports. Because of that, as part of making AS2 support graphql@15 I had to push a version of this package with appropriate peer deps. While I was at it I merged a major memory leak fix that had excellent tests. However, we're removing that dependency from Apollo Server 3 (as well as the rest of the superficial "support" for subscriptions). I'd suggest using graphql-ws rather than subscriptions-transport-ws/graphql-subscriptions for now. We do hope to implement real support for subscriptions in Apollo Server (actually integrated with ApolloServer in a deep way rather than kinda stapled on the side) either via a brand new implementation or via graphql-ws at some point, but for now I'd recommend using the actively maintained subscriptions packages.

I'm tracking figuring out what to do with these projects in apollographql/apollo-server#5141 as part of the AS3 release process.

[n1ru4l]

graphql-subscriptions is actually not a package that is very graphql specific but rather more abstraction for wiring up different EventEmitter like services into AsyncIterables, which graphql-js requires for subscriptions.

I don't know whether you saw my comment over here: #240 (comment)

Maybe this package should be deprecated and instead a EventEmitter abstraction for different systems such as redis pubsub or mqtt could be used.

[n1ru4l]

@glasser You could have considered this for the v2.0.0 release... It is a shame that the typings are still wrong :/

[glasser]

@n1ru4l This package is not actively maintained. 2.0.0 was the bare minimum to get it to build with graphql@16.

[hwillson]

Thanks very much @n1ru4l - looks great!