Move TContext generic from requestDidStart method to ApolloServerPlugin Interface

[jdmoody]

When using an ApolloServerPlugin with a different context, I get the following TypeScript error:

Type 'TContext' is not assignable to type 'Context'.

I believe this is due to annotating the requestDidStart context argument with a different context (in an attempt to get type-safety for the function):

export const MyPlugin: ApolloServerPlugin = {
    requestDidStart(context: GraphQLRequestContext<MyContext>) {

---

After this change, I'll be able to set the context on the plugin, instead:

export const MyPlugin: ApolloServerPlugin<MyContext> = {
    requestDidStart(context) {

If there's another way to accomplish this, please let me know! Thanks!

[apollo-cla]

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

[trevor-scheer]

This LGTM @jdmoody, and thank you for submitting this! I'd like for @abernix to take a peek for a quick sanity check as well.

[abernix]

Thanks for looking at this!

Without re-creating a running example myself, I believe I understand why this is necessary (And while I'm almost certain that providing TContext on ApolloServerPlugin would be more ideal) but wouldn't this be a breaking change for anyone who was currently passing their own context type in the TContext type argument to (the existing) requestDidStart? (Were you able/unable to pass in TContext to requestDidStart in your own implementation of it?)

[jdmoody]

Without re-creating a running example myself, I believe I understand why this is necessary (And while I'm almost certain that providing TContext on ApolloServerPlugin would be more ideal) but wouldn't this be a breaking change for anyone who was currently passing their own context type in the TContext type argument to (the existing) requestDidStart? (Were you able/unable to pass in TContext to requestDidStart in your own implementation of it?)

@abernix, as far as I can tell, there's no way to pass in a type argument to an Interface method when implementing that Interface.

I think the type argument can only be passed in when calling requestDidStart, which is done within apollo-server-core, right?

apollo-server/packages/apollo-server-core/src/requestPipeline.ts

Line 549 in 8059326

if (!plugin.requestDidStart) continue;

---

I initially tried something like this:

import MyContext from 'src/context'

export const MyPlugin: ApolloServerPlugin = {
    requestDidStart<MyContext>(context) {

But this just defines a new generic called MyContext rather than passing in the MyContext type defined above.

[trevor-scheer]

Yeah, this checks out on my end as a fix for something that's currently in a broken state. I recall running into a similar issue.

Minimal reproduction @abernix:
https://bit.ly/2qyEKc2

[MichalLytek]

I've also encountered this problem about not being able to provide custom Context type to the plugin hook.

This change also allows to use inline plugins:

plugins: [
  {
    requestDidStart(requestContext: GraphQLRequestContext<Context>) {
      return {
        willSendResponse() {
          console.log(requestContext.context.requestId);
        },
      };
    },
  },
],

[trevor-scheer]

Bumping this for Monday, cc @abernix.

[abernix]

It's worth noting that what @MichalLytek provided above does work and doesn't sacrifice any type safety from what I can tell — since requestDidStart is early enough in the pipeline where it doesn't have any additional type narrowing applied to its GraphQLRequestContext. It is, however, obviously still not the intuitive approach, so I'm still supportive of landing this.

Furthermore, if we couple this change to this declaration for ApolloServerPlugin on the plugins member of this interface, we may have some type wins?

apollo-server/packages/apollo-server-core/src/requestPipeline.ts

Line 95 in e3d3b90

plugins?: ApolloServerPlugin[];

[trevor-scheer]

Thanks again, @jdmoody!

[dancon]

when to release the latest version? @abernix @trevor-scheer