Skip to content

Conversation

@stephentuso
Copy link

After opening #192, I looked into it more and saw that graphql-js is actually expecting an AsyncIterable, not AsyncIterator, to be returned from the subscribe function: source.

This PR updates the types, methods, and docs to reflect that. This is a breaking change. As mentioned in leebyron/iterall#49, TS can't recognize $$asyncIterator as Symbol.asyncIterator, so type assertions had to be used in a few places

BREAKING CHANGE: PubSubEngine#asyncIterator renamed to asyncIterable,
and return type changed to AsyncIterable
@apollo-cla
Copy link

@stephentuso: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@grantwwu
Copy link
Contributor

So as I'm understanding it, AsyncIterables are objects which you can get an asyncIterator out of using the $$asyncIterator function. So technically our asyncIterators should always have been an asyncIterator in the first place?

So this is a breaking change in the sense that we're changing the types, but all working implementations should already be AsyncIterables, right?

@grantwwu
Copy link
Contributor

Also, this seems like it's related to #147 ...

@stephentuso
Copy link
Author

Oops - didn't do my homework on this one. Yes, this is basically the same as #147 without the changes re. #143. It is breaking because types and method names changed, you are correct though, it looks like all the implementations are already returning AsyncIterables.

Seems like I should probably just close this in favor of #147?

@Akxe
Copy link

Akxe commented Apr 4, 2020

How about this would look into implementing #223 (generic types) too?

@hwillson
Copy link
Member

It looks like the work here has been superseded by #232 (which is coming in 3.0. Let us know if anything is missing, and sorry we didn't get to this contribution sooner!

@hwillson hwillson closed this Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants