search

apollographql / graphql-subscriptions

:newspaper: A small module that implements GraphQL subscriptions for Node.js
MIT License
1.59k stars 133 forks source link

Implemented Generic AsyncIterator #78

Closed ozyman42 closed 5 years ago

ozyman42 commented 7 years ago

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 commented 7 years ago

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

ozyman42 commented 7 years ago

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 commented 7 years ago

Any chance we can get these changes integrated?

jedwards1211 commented 6 years ago

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 commented 5 years ago

Is there any hope for this to get merged?

jedwards1211 commented 5 years ago

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

grantwwu commented 5 years ago

@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 commented 5 years ago

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

grantwwu commented 5 years ago

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 https://github.com/apollographql/graphql-subscriptions/issues/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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/DefinitelyTyped/DefinitelyTyped/pull/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 https://github.com/apollographql/graphql-subscriptions/issues/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 commented 5 years ago

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 commented 5 years ago

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

grantwwu commented 5 years ago

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 commented 5 years ago

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

grantwwu commented 5 years ago

😰

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 commented 5 years ago

@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 commented 5 years ago

@jedwards1211 I sent you an invite to be a collaborator

jedwards1211 commented 5 years ago

@AlexLeung thanks!

jedwards1211 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@AlexLeung interesting, okay sounds good

jedwards1211 commented 5 years ago

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

grantwwu commented 5 years ago

Sorry for the extended delay...

grantwwu commented 5 years ago

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

jedwards1211 commented 5 years ago

I don't mind

ozyman42 commented 5 years ago

I don't mind either.

grantwwu commented 5 years ago

Merged as #198