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

Subscription execution #495

Merged
merged 37 commits into from
Aug 21, 2021
Merged

Conversation

bhoriuchi
Copy link
Contributor

@bhoriuchi bhoriuchi commented Jul 15, 2019

Adds subscription execution support and a Subscribe resolver to Field and FieldDefinition types

Resolves #242

@coveralls
Copy link

coveralls commented Jul 15, 2019

Coverage Status

Coverage decreased (-0.4%) to 91.991% when pulling f51a3a2 on bhoriuchi:subscription-execution into 8a92e97 on graphql-go:master.

@remorses
Copy link
Contributor

Is the done channel necessary? Can’t you just close the channel or cancel the context?

@bhoriuchi
Copy link
Contributor Author

Probably not. I should probably revisit the strategy on the iterator here.

@remorses
Copy link
Contributor

remorses commented May 16, 2020

I looked at the other graphql go libraries implementations and i found out a few things:

  • If the Subscribe function is a method of schema then schema will implement this interface so we can reuse the graphql-transport-ws handler implementation. This handler already implements the websockets messaging logic

  • There is no need for a different Subscribe function for every type, types under the Subscription object will return a channel and error in the Resolver function, this way we can have a similar UX as gqlgen and graphql-gophers, here is an example subscription resolver

  • no need for a done channel, we can use the cancellable context to end the subscription, the resolver will check if the context is cancelled to terminate the goroutine, here is an example

@bhoriuchi
Copy link
Contributor Author

Thanks for the research on this. One of the goals of this project is to follow the official reference implementation. That is the motivation behind some of the existing design choices. I will review these.

Using the context to cancel seems straightforward enough and I was originally using the context to cancel but ran into issues that I cant recall so I will revisit that.

@bhoriuchi
Copy link
Contributor Author

  • There is no need for a different Subscribe function for every type, types under the Subscription object will return a channel and error in the Resolver function, this way we can have a similar UX as gqlgen and graphql-gophers, here is an example subscription resolver

Please see graphql spec https://spec.graphql.org/June2018/#sec-Source-Stream and https://spec.graphql.org/June2018/#sec-Response-Stream. In a subscription the purpose of subscribe is to generate the source stream who's messages are used as the source in the resolve function to generate the response.

  • no need for a done channel, we can use the cancellable context to end the subscription, the resolver will check if the context is cancelled to terminate the goroutine, here is an example

probably fine, I'd like to re-work the iterator anyway.

@bhoriuchi
Copy link
Contributor Author

@remorses I have changed the following

  • Subscribe now takes a context as its first argument. Cancelling this context ends the subscription.
  • Subscribe returns a chan *Result. This remove the use of the ResultIterator type which has been removed from the source code
  • Subscribe function on a type is now expected to return a chan interface{}, this removes the use of the Subscriber type which has been remove from the source code

@remorses
Copy link
Contributor

remorses commented May 18, 2020

I made some changes to your fork, i opened a pull request on your branch here

What i changed

  • split the Subscribe function in Subscribe (similar to graphql.Do) and ExecuteSubscription (similar to graphql.Execute)
  • Subscribe now does the parsing and validation, more similar to graphql.Do
  • removed the added context as there was already one in the params
  • rewritten tests as a grid
  • added testutils for subscriptions tests
  • added a SubscriptableSchema that can be used with https://github.com/graph-gophers/graphql-transport-ws

I still have to check if it works ok with the handler

@remorses
Copy link
Contributor

remorses commented May 18, 2020

I made an example usage repo with playground: https://github.com/remorses/graphql-go-subscription-example

@exapsy
Copy link

exapsy commented Aug 11, 2020

This feature looks great indeed. It solves one of the problems with subscription. Do you guys feel comfortable merging this PR?

@bhoriuchi
Copy link
Contributor Author

bhoriuchi commented Aug 28, 2020

This feature looks great indeed. It solves one of the problems with subscription. Do you guys feel comfortable merging this PR?

I believe so, just waiting for one of the project owners to review and merge. I don't believe there are any breaking changes in any of the code here. Adding a subscribe field is the only modification to object types and the subscription code is all self contained/new code.

@pckilgore
Copy link

I agree that I would love to see this on master. Has anyone reached out directly to @chris-ramon (seems to be the only active maintainer) to see if he is aware of this PR. At this point, the most helpful thing would be reasons from him why it isn't being merged / reviewed.

@bhoriuchi
Copy link
Contributor Author

@pckilgore I have reached out and he has seen the PR.

@jpascal
Copy link

jpascal commented Sep 28, 2020

Any progress here?

I have one question to @bhoriuchi

If I have defined schema like:

schema {
    ...
    subscription: Subscriptions
}
type Subscriptions {
   worked(arg: String): String
   foo: Foo
}
type Foo {
   foo(arg: Strring): String
   bar: Bar
}
type Bar {
   bar(arg: Strring): String
}

Will it work?

subscription { worked(arg: "test") }
subscription { foo { foo(arg: "test") } }
subscription { foo { bar { bar(arg: "test") } } }

I have troubles with last two examples.

@chris-ramon chris-ramon mentioned this pull request Nov 1, 2020
@bhoriuchi
Copy link
Contributor Author

I found an issue with field selections returning nil. Working on a fix. Please do not merge yet. TY

@bhoriuchi
Copy link
Contributor Author

bhoriuchi commented Nov 12, 2020

Never mind. This is working as designed. see apollographql/graphql-subscriptions#51 . The event message needs to be nested in the subscription field name for it to resolve correctly. Merging can proceed

here is an example of a subscribe resolver using a theoretical event emitter that publishes events

func SubscribeFoo(p graphql.ResolveParams) (interface{}, error) {
	topic := "foo"
	c := make(chan interface{})

	off := DefaultEmitter.On(topic, func(msg ...interface{}) {
		data := map[string]interface{}{
			p.Info.FieldName: msg[0],
		}

		c <- data
	})

	go func() {
		for {
			select {
			case <-p.Context.Done():
				off()
				close(c)
				return
			}
		}
	}()

	return c, nil
}

@exapsy
Copy link

exapsy commented Mar 4, 2021

Any updates? I constantly see re-sync with master, but I dont see it ever actually merging with master. This has been going on like from 2019 :P What is going on? :D

@AttilaTheFun
Copy link

@chris-ramon Is there anything still blocking this PR from merging? I see it has been kept alive but it's unclear what the status is at this time.

@bhoriuchi
Copy link
Contributor Author

For those of you following this thread, if you want to use this feature and don't mind using the replace feature in go mod, this is how i have been using it

replace github.com/graphql-go/graphql v0.7.9 => github.com/bhoriuchi/graphql v1.0.0-beta2

@didrikmunther
Copy link

@chris-ramon Will this get merged? What is the status here?

@bhoriuchi
Copy link
Contributor Author

Happy belated 2 year anniversary PR!

@asif-mahmud
Copy link

still waiting for the glorious day when this will be merged :)

Copy link
Member

@chris-ramon chris-ramon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 🚢 — Thanks a lot for the great work & feedback everybody! All kudos to: @bhoriuchi & @remorses.

Confirmed this graphql.Subscribe implementation follows the implementation from graphql-js and works as expected.

Also just for reference, I've put together a small working example integrating this graphql.Subscribe implementation and GraphQL Playground: https://github.com/graphql-go/subscription-example

@chris-ramon chris-ramon merged commit f02a1c9 into graphql-go:master Aug 21, 2021
@bhoriuchi
Copy link
Contributor Author

bhoriuchi commented Aug 22, 2021

Thanks for getting this in @chris-ramon can we possibly get a version (v0.7.10) tag applied?

@chris-ramon
Copy link
Member

chris-ramon commented Aug 29, 2021

Thanks for getting this in @chris-ramon can we possibly get a version (v0.7.10) tag applied?

@bhoriuchi great call! — Just created v0.8.0, released as minor version since we're adding a new end-user API: graphql.Subscribe.

@Juvenal-Yescas
Copy link

Maybe someone needs this ping example

@bhoriuchi bhoriuchi deleted the subscription-execution branch March 30, 2022 22:39
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.

Adding "Subscribe" field to subscription definitions