Implemented Generic AsyncIterator

[ozyman42]

The AsyncIterator takes in a PubSubEngine. I also changed the PubSubEngine from an interface to an abstract class, with all methods being abstract except for the asyncIterator method which is implemented. The advantage of this contribution is that now all different types of Apollo PubSubEngines can share the same AsyncIterator code instead of each having to implement their own version. @davidyaha has brought this contribution into his graphql-redis-subscriptions repository so far, but we both think it would be best to merge the contribution here instead.

[dotansimha]

@alexleung , thank you!
Any change to add some basic unit tests for this PR?

[ozyman42]

I figured that the existing unit tests for the asyncIterator method would suffice since the API for the AsyncIterator and PubSubEngine have not changed.

[ozyman42]

Any chance we can get these changes integrated?

[jedwards1211]

If you guys ask me, we should also extract the core concept of a PushableAsyncIterator (with a public pushValue method) into its own package and make graphql-subscriptions consume it.

[alfaproject]

Is there any hope for this to get merged?

[jedwards1211]

Would like to use this in graphql-multiplex-subscriptions instead of consuming the non-public API from graphql-redis-subscriptions

[grantwwu]

@alfaproject @jedwards1211 Yes, have hope! I in theory have the power to merge this!

I'll try to take a look this week. Do note that I don't work for Apollo and that this does seem like a breaking change... Additionally, I have to coordinate with Apollo for releases; I don't have the ability to cut releases.

[ozyman42]

I can take another look at this in a few days as well.

[grantwwu]

Sorry I took a while to look at this.

This mostly seems fine to me. There's some formatting change noise in the diff, and @NeoPhi's feedback should be pretty easy to address.

Can you add notes to the README under vNEXT or whatever it's called? Also, a rebase or merge to fix the conflicts would be wonderful.

My primary concern is that #143 is still around. @jedwards1211 can you weigh in with a summary of what we know so far? Does using AsyncIterators still cause memory leaks in combination with async generators?

Additionally, I think it would make sense for the publish call to be more permissive in what it takes as parameters to give PubSubEngine implementers more flexibility.

By the way, we should note that this shouldn't break existing implementations, due to how Typescript handles abstract classes (i.e. allows treating them as interfaces). https://stackoverflow.com/questions/35990538/extending-vs-implementing-a-pure-abstract-class-in-typescript

[ozyman42]

• I merged to fix the conflicts then addressed @NeoPhi's feedback.
• I modified the generic async iterator to satisfy the new test case where event listeners should not be added until next() is called for the first time (see 79d7f08).
• I'm not sure what you would like added to README about this.
• In order to use this generic async iterator, implementers must use extends PubSubEngine rather than implements PubSubEngine, if they use implements PubSubEngine then TypeScript will require them to implement the asyncIterator method themselves.
• Compiling was not working at first because of a problem with the sinon types repo; see [@types/sinon] version 5.0.6: error TS2370: A rest parameter must be of an array type. DefinitelyTyped/DefinitelyTyped#30657 so I changed the package.json to use exactly version 5.0.2 ("@types/sinon": "5.0.2") instead of semver compatible with version 5.0.1 ("@types/sinon": "^5.0.1") which fixed the issue.
• Perhaps publish parameters could be added in a different PR

I can rebase instead if it makes it easier to review.

I noticed that a new test case was added which requires that no listeners be added until next() is called for the first time. I updated the generic async iterator implementation to conform to this requirement, but I am wondering why that behavior is preferable. Shouldn't we want incoming events to be pooled immediately after the async iterator is created in case a client of the PubSubEngine wants to run some logic before awaiting the next event? Having async iterators not queue events immediately also seems to violate the invariant that the listening boolean seems to represent.

[grantwwu]

I'm not sure what you would like added to README about this.

Sorry, mind fart - I meant the changelog.

In order to use this generic async iterator, implementers must use extends PubSubEngine rather than implements PubSubEngine, if they use implements PubSubEngine then TypeScript will require them to implement the asyncIterator method themselves.

But they already have to, right? So no existing code should break, they just wouldn't be using your implementation.

Compiling was not working at first because of a problem with the sinon types repo ...

Seems like this was fixed in DefinitelyTyped/DefinitelyTyped#30856, I'll make a note to check whether a fix is released or not by the time we release this.

I noticed that a new test case was added which requires that no listeners be added until next() is called for the first time. I updated the generic async iterator implementation to conform to this requirement, but I am wondering why that behavior is preferable. Shouldn't we want incoming events to be pooled immediately after the async iterator is created in case a client of the PubSubEngine wants to run some logic before awaiting the next event? Having async iterators not queue events immediately also seems to violate the invariant that the listening boolean seems to represent.

See #143 - the concern was that certain use patterns could cause listeners to leak. The listening boolean isn't part of the public interface... I'm not quite sure, honestly, what it was intended to mean.

Do you have any use cases in mind re: a client wanting to run some logic before awaiting the next event? I'm personally having difficulty thinking of any where immediately calling .next and then ignoring the result wouldn't work.

[ozyman42]

I meant the changelog.

You mean documenting the updates made here for the next release? I don't have access to release drafting. If I did I would summarize the change as:

Replaced eventEmitterAsynIterator with default generic AsyncIterator which is automatically used by any PubSubEngine implementation which opts to use extends PubSubEngine rather than implements PubSubEngine. No breaking changes for those who continue to use implements PubSubEngine. #78

difficulty thinking of any where immediately calling .next and then ignoring the result wouldn't work.

This can work. My worry is just that the requirement to do this to achieve the behavior of an immediately subscribed AsyncIterator seems a little unintuitive, and should definitely be documented somewhere. This would be best addressed in a separate issue/PR.

[ozyman42]

I updated the PubSub Implementations section of README so it's up-to-date with the changes made.

[grantwwu]

I meant https://github.com/apollographql/graphql-subscriptions/blob/master/CHANGELOG.md

Sorry, work is super busy again I will look at this again as soon as I have the time

[jedwards1211]

@grantwwu I'd love to see this get merged, do you mind taking a look at it again?

[grantwwu]

😰

Every time I go to GitHub right now I feel a twinge of guilt at not keeping up with my commitments here...

I'll try to make time.

In the meantime, do you think you could fix the merge/rebase conflicts?

[jedwards1211]

@grantwwu sure!

ah wait, I can't rebase and push commits to this; I could rebase and open a new PR if you want

[ozyman42]

@jedwards1211 I sent you an invite to be a collaborator

[jedwards1211]

@alexleung thanks!

[jedwards1211]

@alexleung @grantwwu hey guys, I mainly use Flow, not TypeScript, but...won't changing PubSubEngine from an interface to an abstract class force users to change their code from implements to extends? I think we could avoid a breaking change by keeping PubSubEngine as an interface and exporting a new AbstractPubSubEngine that provides the asyncIterator implementation as Alex Leung intended. It hearkens back to Java madness, but I think it's better to give people the option of extending a class rather than forcing them to extend it...

[ozyman42]

In TypeScript, abstract classes can be implemented or extended. When a subclass implements an abstract class, it must implement all of the public methods of the abstract class, including those that the abstract class implements itself. If a subclass extends an abstract class, then it inherits any methods the abstract class implements. Therefore this is not a breaking change.

[jedwards1211]

@alexleung interesting, okay sounds good

[jedwards1211]

@grantwwu @alexleung okay, I rebased and very carefully merged in the logic to not subscribe until the first call to next()

[grantwwu]

Sorry for the extended delay...

[grantwwu]

Do either of you (@alexleung or @jedwards1211) mind if I merge a version that I've fixed up myself?

[jedwards1211]

I don't mind

[ozyman42]

I don't mind either.

[grantwwu]

Merged as #198