Additional extensions support

[sebas5384]

Passing additional extensions (other than the default Tracing and Cache Control) seems to be a need (#657) which could open the door to new and interesting extensions.

TODO:

• Update CHANGELOG.md with your change (include reference to issue & this PR)
• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass

[apollo-cla]

@sebas5384: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[martijnwalraven]

Yep, I think the ability to pass in additional extensions is great. The reason we haven't done that so far is that we didn't want to commit to the current GraphQLExtension API. So maybe we can do a quick pass to see what makes sense to change there.

One change we discussed is to get rid of the separate ...didStart/...didEnd lifecycle methods in favor of a callback pattern. So you'd have requestDidStart: (() => void) | void and the callback would be invoked on end.

What do you think? How are you using the extension API? Anything else you're missing or would like to improve about it?

[sebas5384]

Hi @martijnwalraven !
Yeah I thought about the current robustness of GraphQLExtension API and I agree.
Don't know exactly what you mean by:

So you'd have requestDidStart: (() => void) | void and the callback would be invoked on end.
Could you give me an example?

I'm missing some contextual arguments on the lifecycle methods in order to avoid unnecessary statefulness inside the extension instance. For example, requestDidEnd() could pass the documentAST (parsed query), schema and the context object as arguments, in order to decide given the operation type (mutation or query) what should do with the response from the request, in my case adding a header.

Today, I would have to save a reference of the request response inside the extension when the first field will be resolved, though instead I could do it inside requestDidEnd() method which makes more sense to me.

[martijnwalraven]

The idea behind the callback pattern would be that you could keep start and end together:

  requestDidStart() {
    this.startWallTime = new Date();
    this.startHrTime = process.hrtime();
    return () => {
          this.duration = process.hrtime(this.startHrTime);
          this.endWallTime = new Date();
    };
  }

That would also allow you to use local state when needed. Note that right now a new instance of the GraphQLExtension class is created for every request though.

I'm also wondering where you're adding a header, because the current API is meant to add GraphQL extension data and doesn't give you access to the underlying response.

What is your use case exactly? If we think adding headers is common, maybe we should add the equivalent to format() to modify the response. One problem with that is that when transport batching or WebSockets are used, there is no one-to-one correspondence between a GraphQL response and an HTTP response.

[sebas5384]

@martijnwalraven I guess the main benefit I see with callbacks is local scope, for example, the requestDidStart() and requestDidEnd() could share scoped variables so in that way you remove the need of this.someStateVar to share values between a start and end from a request. But we should be careful to avoid callback hell here.

Here's what I would love to do with Apollo Server:
https://www.drupal.org/docs/8/api/cache-api/cache-tags
but for that I need the followed:

1. Create an instance collector which will contain the bag of cache-tags and inject it in context.
2. Wrap every field resolver after and before it resolves in order to collect automatically the ObjectType:ID from the result. Will need the info object for that in order to get the returnType and add the ID field in case is not present at the query.
3. Field resolvers can manually add cache tags by using the collector from the context. Ex.: ctx.cacheTags.addTags(['latest-articles']).
4. Decide given the query operation type (mutation or query) what to do at the end of the request.
   4.1. Query: Add all the collected cache tags inside a header like Cache-Tag: article:1, article:2.
   4.2. Mutation: Purge all the collected cache tags. Ex.: Integration with CDN's API's, etc...

PS: Probably I could write a middleware at the web server level and parse the query, schema, etc... but thats an overhead in my opinion which I could solve it nicely inside the apollo-server as an extension (I actually prefer the middleware concept).

For now, I guess if the response is passed to the format() method would be a quick solution 👍

One problem with that is that when transport batching or WebSockets are used, there is no one-to-one correspondence between a GraphQL response and an HTTP response.

Yeah, that's why I would prefer to have a lifecycle for the request, and not the execution of the query (maybe I wrong about it). Someone already touch on this matter: #657 (comment)

I see why this PR could take a while to be merge since is about two projects, graphql-extensions and apollo-server (and probably graphql-yoga which uses apollo-server). Maybe we could deliver the extensions option and grown from there.

Hope I'm helping 😄

[abernix]

I guess the main benefit I see with callbacks is local scope, for example, the requestDidStart() and requestDidEnd() could share scoped variables so in that way you remove the need of this.someStateVar to share values between a start and end from a request. But we should be careful to avoid callback hell here.

I really think this is a particularly nice benefit!

[richardwillars]

I wonder if the tracing extension should be applied before all other extensions, otherwise it's going to give misleading numbers (the custom extensions may take a long time to execute, but the tracing would never pick up on that as it'd start recording AFTER they've all executed)

[glasser]

Great idea. We should also run "didStop" handlers in reverse order for that reason.

[richardwillars]

Just wondered if there's any update or further thought on getting this merged in? Got a few scenarios where custom extensions would fit the bill nicely.

[glasser]

I'm working on this this week, along with improving the extensions API.

[glasser]

I've rolled this into #1105. Thanks!

[evans]

Thank you so much @sebas5384 for the initial effort!