Add hooks to Mercurius

[jonnydgreen]

Closes: #380

[mcollina]

I've done a quick pass, I need to get deeper but I think there are a few things to fix already.

[mcollina]

Good work! How do the hooks play with subscriptions?

[jonnydgreen]

Thanks! I have tested on a very basic level and seems to be playing okay. I'll write some thorough tests for subscriptions for both normal server and gateway mode to make sure it is all working as expected :) I also noticed that the CI checks are intermittently failing for some of the subscription tests. At first glance it seems to be an environmental issue (other checks for the same node version pass fine) - did you have any thoughts on this? (Example run: https://github.com/mercurius-js/mercurius/pull/385/checks?check_run_id=1791951386)

[mcollina]

I've seen the flaky tests... can't do much about it right now. If you'd like to investigate and fix them, I'd be very grateful!

[jonnydgreen]

For sure, very happy to take a look at the tests! :)

[mcollina]

Good work! I did another pass and I've left a few comments.

[mcollina]

Good work, one last nit.

[mcollina]

Could you link the new docs pages from the README as well?

[jonnydgreen]

Could you link the new docs pages from the README as well?

Sure thing!

Good work! How do the hooks play with subscriptions?

After a bit of experimentation, I think we need to add a few more hook events within lib/subscription-connection.js. From my observations with subscriptions so far, I have seen that preGatewayExecution is executed when using federated subscriptions. We don't see any other hooks triggered because subscriptions don't go through the normal request route. To fix this, I suggest we add preParsing, preExecution and onResolution (not preValidation because we don't do any validation in subscriptions) to lib/subscription-connection.js to make sure we capture the full GraphQL lifecycle for subscriptions.

My only concern with this approach is the meaning of context when applied to the hooks. Because it is not strictly associated with a request over the wire, one could argue that we should have subscription specific hooks for this situation instead. I wondered what your thoughts were on this?

[mcollina]

lgtm

[mcollina]

My only concern with this approach is the meaning of context when applied to the hooks. Because it is not strictly associated with a request over the wire, one could argue that we should have subscription specific hooks for this situation instead. I wondered what your thoughts were on this?

It's better to have a subscription specific hook, ideally in a follow up pull request. This is good to ship right now, so I'm shipping it as a few people are waiting on this!

[jonnydgreen]

Yep, makes sense - happy to do a follow up PR for this as you suggested! Thanks for shipping this afternoon! :)

[SimenB]

should this be fastify.graphql.addHook? only the first example in this document has that, not the others

[jonnydgreen]

You're correct, yep - I'll submit a PR to fix this over lunch! Unless you want to! :)