Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Class-based subscription fields #1930

Merged
merged 13 commits into from
Jan 25, 2019
Merged

Class-based subscription fields #1930

merged 13 commits into from
Jan 25, 2019

Conversation

rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Oct 29, 2018

The current subscription system leaves a lot to be desired, see for example #1528 #1799 #1910

I'm proposing a GraphQL::Schema::Subscription like GraphQL::Schema::Mutation with authorization, initialization, and update methods.

You can see the rendered doc here: https://github.com/rmosolgo/graphql-ruby/blob/good-subscriptions/guides/subscriptions/subscription_classes.md

TODO:

  • What if you don't want to update a certain client of an event? (For example, that client triggered the event)

@theorygeek
Copy link
Contributor

theorygeek commented Oct 29, 2018

Nice! I like it. How do you hook up the transport? I'm assuming there needs to be some method that GraphQL can invoke when an update needs to be sent to a client?

Then, def update will be called for each client's subscription

It is going to send the update to every client by default, and your update should return skip if that particular client doesn't need the update?

@rmosolgo
Copy link
Owner Author

Thanks for taking a look!

How do you hook up the transport?

That consideration is separated a bit. The changes here revolve around how subscriptions are evaluated by GraphQL, but not how the result is transmitted to clients.

In terms of transport, the subscription implementation should define #deliver for sending payloads to clients. See ActionCable's #deliver for example.

every client by default?

Yes, that's right. I'm glad you mentioned it: it should really have a way to halt, and send nothing to a client if desired, but there's not a way to do that yet. I added a TODO above.

@modosc
Copy link
Contributor

modosc commented Nov 9, 2018

What if you don't want to update a certain client of an event?

we've hit this a few different times - mostly because of my understanding on how scoping works it's hard to have multidimensional filters (ie, company A + role B). we sometimes end up sending out notifications on a model which is not visible for some clients. if we allow the subscription field to be null, then we send out empty messages to a bunch of clients. my hacky solution was to make the field non-nullable and hide the Cannot return null for non-nullable field Subscription.foo error but this isn't great. ideally there'd be some option for "not null for the client but if the server returns null just skip delivery".

@rmosolgo rmosolgo mentioned this pull request Jan 21, 2019
19 tasks
@rmosolgo rmosolgo added this to the 1.9.0 milestone Jan 22, 2019
@rmosolgo rmosolgo changed the base branch from 1.9-dev to master January 23, 2019 11:13
@rmosolgo
Copy link
Owner Author

It's basically implemented as written, feel free to take another look if you're interested! Otherwise I'll merge it tomorrow or so.

@rmosolgo rmosolgo merged commit 9f21537 into master Jan 25, 2019
@rmosolgo rmosolgo deleted the good-subscriptions branch January 25, 2019 20:12
@phyllisstein
Copy link

phyllisstein commented Jan 26, 2019

I'm so excited about this @rmosolgo, thank you for all your amazing work. ✨

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.

4 participants