Closed n1ru4l closed 2 years ago
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.
@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 😇
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.
@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...
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.
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: https://github.com/apollographql/graphql-subscriptions/issues/240#issuecomment-767568596
Maybe this package should be deprecated and instead a EventEmitter abstraction for different systems such as redis pubsub or mqtt could be used.
@glasser You could have considered this for the v2.0.0 release... It is a shame that the typings are still wrong :/
@n1ru4l This package is not actively maintained. 2.0.0 was the bare minimum to get it to build with graphql@16.
BREAKING fixes the type annotation of the abstract class PubSubEngine. According to the TypeScript type-defintion a
PubSubAsyncIterator
instance is actually anAsyncIterableIterator
instead of anAsyncIterator
. The typing ofPubSubAsyncIterator
is way more convenient as it allows iterating over it with thefor 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 https://github.com/apollographql/graphql-subscriptions/issues/192