Pass SubscriptionOptions to PubSubEngine.subscribe

[quintstoffers]

See /issues/7

By passing the SubscriptionOptions to the PubSubEngine it can in turn create more specific subscriptions to sources such as RabbitMQ and rethinkDB.

For example, assume that we are interested in updates to a specific comment. We could listen to all comment updates and use our filter function to only send the relevant updates to the client. However, this assumes that it's feasible to listen to all comment updates on our Apollo server in the first place.

By allowing PubSubEngine.subscribe access to SubscriptionOptions it could listen for exactly the subset of comment updates the client requested. For instance, by subscribing to the RabbitMQ channel updates.comments.COMMENT_ID as opposed to updates.comments.

[helfer]

@quintstoffers I like the idea of passing the options to the subscribe method, but I'm a little concerned about passing everything, because I feel like it would tightly couple the PubSub subscribe function to GraphQL-specific things like the query and variables. I don't think PubSub should deal with that kind of stuff.

Can you give an example of how you would use this? Could we maybe cover this by adding some functionality to setupFunctions, which would then also return a value that's used to pass to subscribe?

[quintstoffers]

@helfer I agree with what you're saying. Passing the entirety of SubscriptionOptions to PubSub is overkill. However, submitting a PR was suggested on Slack (#subscriptions) and I figured it would be a good way to to bring issue #7 to everyone's attention.

I like the idea of expanding setupFunctions. Perhaps we can pass Objects instead of functions so that we can define exactly what we want our channel to do, and what we want to do with messages that come in through it.

Perhaps something like this would work. The code example below would ask PubSub to only listen for comment updates with the given repoFullName:

{
    setupFunctions: {
        // Taken from GitHunt.
        commentAdded: (options, args) => ({
            commentAdded: comment => comment.repository_name === args.repoFullName
        }),
        // Proposed structure.
        commentUpdated: (options, args) => ({
            commentUpdated: {
                // These options could be passed to PubSub.
                channelOptions: {
                    repoFullName: args.repoFullName
                },
                // Technically redundant since we specified repoFullName above and thus should
                // only receive relevant messages. However, it's here for completeness' sake
                filter: comment => comment.repository_name === args.repoFullName
            }
        })
    }
}

Thanks to channelOptions, pubSub.subscribe can create a channel that will efficiently listen for comment updates in a specific repository, instead of being forced to listen to and emit all comment updates.

By passing an Object we can also expand in the future. In fact, I'd be very interested in being able to specify a reduce function there as well so that we can convert the message payload after it has passed through the filter function, but that's something for another day 😄 .

[helfer]

@quintstoffers That sounds good. If anything, I think it should always return an object, and make filter optional as well (in which case it will default to true). Would you have time to make a PR with that change?

For converting the message payload, you can actually use the resolve function of the subscription, which usually does nothing other than pass through the rootValue. That's assuming the payload looks the same for all your channels. If it doesn't, we might indeed want to add something like that. But let's keep that for a later time.

[quintstoffers]

@helfer I'm pretty sure I'll be able to have a PR some time next week. I hope my TypeScript is up to scratch 😉.

Do you think we should support the "old" format that returned a function and thus be backwards compatiblr? We can detect the function type and convert it to an object with the filter property.

[helfer]

@quintstoffers: Great, I look forward to it!

As for backwards compatibility, I don't think we should have that. It's too early in the project to incur the kind of technical debt that backwards compatibility usually brings with it.

[quintstoffers]

@helfer: See #11.