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

Add Subscribe function to make easier to create Subscription Handlers #530

Closed
wants to merge 1 commit into from

Conversation

racerxdl
Copy link
Contributor

@racerxdl racerxdl commented Feb 25, 2020

I added Subscribe function to Object / Fields Definitions so its easier to make GraphQL Subscription Handlers. I only added the field so it wouldn't break compatibility and if you don't use it, it wont make a difference.

So for example in https://github.com/racerxdl/go-subscription-handler I did:

var rootSubscriptions = graphql.ObjectConfig{
    Name: "RootSubscriptions",
    Fields: graphql.Fields{
        "serverTime": &graphql.Field{
            Type: graphql.String,
            Resolve: func(p graphql.ResolveParams) (interface{}, error) {
                err := subhandler.Subscribe(p.Context, "serverTime")

                if p.Source != nil {
                    // We received a event data
                    v, ok := p.Source.(map[string]interface{})
                    if ok && v["time"] != nil {
                        return v["time"], nil
                    }
                }

                // We didn't receive a event data, so resolve normally
                return time.Now().String(), err
            },
        },
    },
}

With that Subscribe field in the struct I could do:

var rootSubscriptions = graphql.ObjectConfig{
    Name: "RootSubscriptions",
    Fields: graphql.Fields{
        "serverTime": &graphql.Field{
            Type: graphql.String,
            Subscribe: func(p graphql.ResolveParams) error {
                return subhandler.Subscribe(p.Context, "serverTime")
            },
            Resolve: func(p graphql.ResolveParams) (interface{}, error) {
                if p.Source != nil {
                    // We received a event data
                    v, ok := p.Source.(map[string]interface{})
                    if ok && v["time"] != nil {
                        return v["time"], nil
                    }
                }

                // We didn't receive a event data, so resolve normally
                return time.Now().String(), err
            },
        },
    },
}

That should help with #242

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 92.379% when pulling c9a98c0 on racerxdl:master into 02caa89 on graphql-go:master.

@racerxdl
Copy link
Contributor Author

@chris-ramon that's a quick hack to be able to do better subscriptions. There are other PRs about it (like #495 ) but this one is pretty simple and fast to review. I just add a Subscription parameter so the handler can use it and have better code.

@nic
Copy link

nic commented Feb 28, 2020

Nice job!! Any prediction of when it will be accept/merged?

@jeduardocosta
Copy link

I'm also interested in this change. Any news?

@remorses
Copy link
Contributor

Instead of using a different subscribe functions we should use a channel to send events and return this channel as resolver return value

Gqlgen and graphql gophers already do this,

I don’t like the approach of this pr, it is not intuitive and difficult to integrate with pubsub libs

@@ -368,12 +369,15 @@ type IsTypeOfFn func(p IsTypeOfParams) bool

type InterfacesThunk func() []*Interface

type SubscribeFn func(p ResolveParams) error
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the same function signature as a FieldResolveFn, any reason not to just use that?

Copy link

Choose a reason for hiding this comment

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

Wouldn't it be more user friendly to assign this under a name which includes "Subscription" in it? Like, if you wanna search for the particular subscription handler for subscription, its easy to identify it. Could be wrong.

@bhoriuchi
Copy link
Contributor

Instead of using a different subscribe functions we should use a channel to send events and return this channel as resolver return value

Gqlgen and graphql gophers already do this,

I don’t like the approach of this pr, it is not intuitive and difficult to integrate with pubsub libs

I gave a longer answer in my PR but the short version is that this project follows the reference implementation which has this method https://github.com/graphql/graphql-js/blob/master/src/type/definition.js#L1001 . It it also not difficult to integrate into pubsub libs if you implement the Subscribe handler https://github.com/graphql/graphql-js/blob/master/src/subscription/subscribe.js#L63

@remorses
Copy link
Contributor

remorses commented May 18, 2020

ok i think we can close this in favour of #495

@racerxdl
Copy link
Contributor Author

I agree @remorses , Closing in favor of #495

@racerxdl racerxdl closed this Sep 30, 2020
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.

7 participants