create separate pubsub.asyncIterable method and deprecate pubsub.asyncIterator #147
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 anAsyncIterator
; rather, they should return anAsyncIterable
, and event listener registration/anything else necessitating cleanup should only be performed when thatAsyncIterable
'sSymbol.asyncIterator
method is actually called, sincefor await
and probably most 3rd-party code is only guaranteed toreturn
the iterator if it ends up starting iteration (i.e. callingSymbol.asyncIterator
).As a solution, I've created a new
pubsub.asyncIterable
method that we should advise people to use. I also madeeventEmitterToAsyncIterator
print a deprecation warning when it's used like anAsyncIterable
(which will happen with the current recommended usage ofpubsub.asyncIterator
).The
withFilter
method still needs to be redesigned to operate onAsyncIterable
s instead ofAsyncIterators
.