From a926b7eedbb87abab2ec70fb03d71743985cb18d Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Fri, 8 May 2020 18:08:36 +0000 Subject: [PATCH 1/2] Use an object, rather than positional params for `willResolveField`. This should prove to be more ergonomic, and also allow us to attach other useful properties to this object (for either external or internal use) in the future. Also, adds a test to make sure we're passing _something_! Ref: https://github.com/apollographql/apollo-server/pull/3998#discussion_r414900290 --- .../src/__tests__/runQuery.test.ts | 37 +++++++++++++++++++ .../src/utils/schemaInstrumentation.ts | 2 +- .../apollo-server-plugin-base/src/index.ts | 5 ++- packages/apollo-server-types/src/index.ts | 22 +++++++++++ 4 files changed, 63 insertions(+), 3 deletions(-) diff --git a/packages/apollo-server-core/src/__tests__/runQuery.test.ts b/packages/apollo-server-core/src/__tests__/runQuery.test.ts index 228ea74f46d..53f31e64e17 100644 --- a/packages/apollo-server-core/src/__tests__/runQuery.test.ts +++ b/packages/apollo-server-core/src/__tests__/runQuery.test.ts @@ -691,6 +691,43 @@ describe('runQuery', () => { expect(executionDidEnd).toHaveBeenCalledTimes(1); }); + it('receives an object with the resolver arguments', async () => { + const willResolveField = jest.fn(); + const executionDidEnd = jest.fn(); + const executionDidStart = jest.fn( + (): GraphQLRequestExecutionListener => ({ + willResolveField, + executionDidEnd, + }), + ); + + await runQuery({ + schema, + context: { ourSpecialContext: true }, + queryString: '{ testString }', + plugins: [ + { + requestDidStart() { + return { + executionDidStart, + }; + }, + }, + ], + request: new MockReq(), + }); + + expect(executionDidStart).toHaveBeenCalledTimes(1); + expect(willResolveField).toHaveBeenCalledTimes(1); + expect(willResolveField).toHaveBeenNthCalledWith(1, { + source: undefined, + args: {}, + context: expect.objectContaining({ ourSpecialContext: true }), + info: expect.objectContaining({ fieldName: 'testString' }), + }); + expect(executionDidEnd).toHaveBeenCalledTimes(1); + }); + it('calls the end handler', async () => { const didResolveField: GraphQLRequestListenerDidResolveField = jest.fn(); diff --git a/packages/apollo-server-core/src/utils/schemaInstrumentation.ts b/packages/apollo-server-core/src/utils/schemaInstrumentation.ts index 41d3f72dd3c..119320b3de9 100644 --- a/packages/apollo-server-core/src/utils/schemaInstrumentation.ts +++ b/packages/apollo-server-core/src/utils/schemaInstrumentation.ts @@ -51,7 +51,7 @@ function wrapField(field: GraphQLField): void { // resolution is complete. const didResolveField = typeof willResolveField === 'function' && - willResolveField(source, args, context, info); + willResolveField({ source, args, context, info }); const resolveObject: GraphQLObjectResolver< any, diff --git a/packages/apollo-server-plugin-base/src/index.ts b/packages/apollo-server-plugin-base/src/index.ts index 448890848a6..91fedfcd2ca 100644 --- a/packages/apollo-server-plugin-base/src/index.ts +++ b/packages/apollo-server-plugin-base/src/index.ts @@ -7,6 +7,7 @@ import { GraphQLResponse, ValueOrPromise, WithRequired, + GraphQLFieldResolverParams, GraphQLRequestContextDidResolveSource, GraphQLRequestContextParsingDidStart, GraphQLRequestContextValidationDidStart, @@ -16,7 +17,6 @@ import { GraphQLRequestContextExecutionDidStart, GraphQLRequestContextWillSendResponse, } from 'apollo-server-types'; -import { GraphQLFieldResolver } from "graphql"; // We re-export all of these so plugin authors only need to depend on a single // package. The overall concept of `apollo-server-types` and this package @@ -34,6 +34,7 @@ export { GraphQLResponse, ValueOrPromise, WithRequired, + GraphQLFieldResolverParams, GraphQLRequestContextDidResolveSource, GraphQLRequestContextParsingDidStart, GraphQLRequestContextValidationDidStart, @@ -102,6 +103,6 @@ export interface GraphQLRequestExecutionListener< > extends AnyFunctionMap { executionDidEnd?: GraphQLRequestListenerExecutionDidEnd; willResolveField?( - ...fieldResolverArgs: Parameters> + fieldResolverParams: GraphQLFieldResolverParams ): GraphQLRequestListenerDidResolveField | void; } diff --git a/packages/apollo-server-types/src/index.ts b/packages/apollo-server-types/src/index.ts index a4ef63ed921..9dc59c5ffac 100644 --- a/packages/apollo-server-types/src/index.ts +++ b/packages/apollo-server-types/src/index.ts @@ -7,6 +7,7 @@ import { OperationDefinitionNode, DocumentNode, GraphQLError, + GraphQLResolveInfo, } from 'graphql'; // This seems like it could live in this package too. @@ -147,6 +148,27 @@ export type Logger = { error(message?: any): void; } +/** + * This is an object form of the parameters received by typical + * `graphql-js` resolvers. The function type is `GraphQLFieldResolver` + * and normally uses positional parameters. In order to facilitate better + * ergonomics in the Apollo Server plugin API, these have been converted to + * named properties on the object using their names from the upstream + * `GraphQLFieldResolver` type signature. Ergonomic wins, in this case, + * include not needing to have three unused variables in scope just because + * there was a need to access the `info` property in a wrapped plugin. + */ +export type GraphQLFieldResolverParams< + TSource, + TContext, + TArgs = { [argName: string]: any } +> = { + source: TSource; + args: TArgs; + context: TContext; + info: GraphQLResolveInfo; +}; + export type GraphQLRequestContextDidResolveSource = WithRequired, | 'metrics' From b7ea44792af147a9fad6349537753ea9184788eb Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Fri, 8 May 2020 18:12:36 +0000 Subject: [PATCH 2/2] Remove unreachable code after `callTargets` decomposition. The `callTargets` method was introduced in 4fa6ddd4f8d, but I forgot to remove the code it rendered unnecessary/aimed to replace. --- packages/apollo-server-core/src/utils/dispatcher.ts | 8 -------- 1 file changed, 8 deletions(-) diff --git a/packages/apollo-server-core/src/utils/dispatcher.ts b/packages/apollo-server-core/src/utils/dispatcher.ts index 02789591abf..0ea761700d3 100644 --- a/packages/apollo-server-core/src/utils/dispatcher.ts +++ b/packages/apollo-server-core/src/utils/dispatcher.ts @@ -29,14 +29,6 @@ export class Dispatcher { return await Promise.all( this.callTargets(this.targets, methodName, ...args), ); - return await Promise.all( - this.targets.map(target => { - const method = target[methodName]; - if (method && typeof method === 'function') { - return method.apply(target, args); - } - }), - ); } public invokeHookSync(