Fix #894, add perEventContextResolver for subscribe

[tgriesser]

Fixes #894

Adds ~contextValueExecution~ perEventContextResolver, a new option for subscribe which allows you to derive a new context value for each independent execution of a subscription.

It takes the original context value as an argument, so you can derive the new context from the original.

[IvanGoncharov]

@tgriesser I like this solution and I think it is generic enough to be added into lib. However, I think contextValueExecution is a confusing name but I can't suggest anything better 🤷‍♂ It's the only blocking issue I have.

Also please update JSDoc comments for relevant functions.

[tgriesser]

Nice! The idea with the name was to take contextValue, which is the name for the original context, combined with the execute, called in mapSourceToResponse.

Maybe one of:

• executeContext
• executeContextValue
• contextValueExecute
• contextValueForExecute

Also please update JSDoc comments for relevant functions.

Sounds good, will do once we land on the name.

[IvanGoncharov]

combined with the execute, called in mapSourceToResponse.

@tgriesser The fact that we call execute under the hood is an implementation detail. Also since it's a function it should be reflected in the name.

Spec describes "stream of events" so I think it should be something like perEventContextResolver. http://spec.graphql.org/June2018/#sec-Subscription

[tgriesser]

The fact that we call execute under the hood is an implementation detail. Also since it's a function it should be reflected in the name.

Ah right, that makes sense.

Spec describes "stream of events" so I think it should be something like perEventContextResolver.

Sounds great.

[tgriesser]

@IvanGoncharov docs should be updated, let me know if you see anything that needs correction.

[IvanGoncharov]

@tgriesser Thanks for the update, now it looks perfect 👍 But I don't want to increase scope of 15.0.0 so will merge after release.

[andreialecu]

Just ran into the issue at #894

Seems that the milestone was missed. Anything holding this back?

[97amarnathk]

@IvanGoncharov @tgriesser I have a similar use case while implementing graphql subscriptions with dataloaders. Any particular reason why this isn't merged yet?

[benjie]

Is there a way to "release" the context? Say, for example, that the context were to contain an authenticated client connected to a datastore.

E.g. for normal GraphQL execution you can do:

function getContext(...) {
  const client = ...;
  return {
    contextValue: { client },
    release: () => client.release(),
  };
}

const { contextValue, release } = getContext(...);
try {
  return await graphql({schema, source, contextValue})
} finally {
  release();
}

[tgriesser]

@benjie that's a really good point... the current change wouldn't support that case well.

In thinking about it more, and looking through the subscribe / createSourceEventStream code, I feel a better option is to add a perEventExecute, so you can tap into / modify anything you'd need about the execution. That way you can do something like:

const iterator = await subscribe({
  schema,
  document,
  variableValues,
  contextValue: new DataContext(),
  operationName: obj.request.name,
  perEventExecute: async (execArgs) => {
    const { contextValue, release } = getContext(...);
    try {
      return await execute({...execArgs, contextValue})
    } finally {
      release();
    }
  }
})

@IvanGoncharov thoughts on that? Something like:

export type SubscriptionArgs = {|
  schema: GraphQLSchema,
  document: DocumentNode,
  rootValue?: mixed,
  contextValue?: mixed,
  variableValues?: ?{ +[variable: string]: mixed, ... },
  operationName?: ?string,
  fieldResolver?: ?GraphQLFieldResolver<any, any>,
  subscribeFieldResolver?: ?GraphQLFieldResolver<any, any>,
+ perEventExecute?: (args: ExecutionArgs) => PromiseOrValue<ExecutionResult>
|};

function subscribeImpl(
  args: SubscriptionArgs,
): Promise<AsyncGenerator<ExecutionResult, void, void> | ExecutionResult> {
  const {
    schema,
    document,
    rootValue,
    contextValue,
    variableValues,
    operationName,
    fieldResolver,
    subscribeFieldResolver,
+   perEventExecute = execute
  } = args;

  const mapSourceToResponse = (payload) =>
-   execute({
+   perEventExecute({
      schema,

[97amarnathk]

@IvanGoncharov Are there any plans to merge this ? If not, then what is the best way to generate a context for each execution currently?

[97amarnathk]

PR which solves this in another way: https://github.com/graphql/graphql-js/pull/3071