New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Properly name the root field execution functions #3294
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,6 +114,7 @@ export interface ExecutionContext { | |
variableValues: { [variable: string]: unknown }; | ||
fieldResolver: GraphQLFieldResolver<any, any>; | ||
typeResolver: GraphQLTypeResolver<any, any>; | ||
subscribeFieldResolver: GraphQLFieldResolver<any, any>; | ||
errors: Array<GraphQLError>; | ||
} | ||
|
||
|
@@ -151,6 +152,7 @@ export interface ExecutionArgs { | |
operationName?: Maybe<string>; | ||
fieldResolver?: Maybe<GraphQLFieldResolver<any, any>>; | ||
typeResolver?: Maybe<GraphQLTypeResolver<any, any>>; | ||
subscribeFieldResolver?: Maybe<GraphQLFieldResolver<any, any>>; | ||
} | ||
|
||
/** | ||
|
@@ -164,46 +166,30 @@ export interface ExecutionArgs { | |
* a GraphQLError will be thrown immediately explaining the invalid input. | ||
*/ | ||
export function execute(args: ExecutionArgs): PromiseOrValue<ExecutionResult> { | ||
const { | ||
schema, | ||
document, | ||
rootValue, | ||
contextValue, | ||
variableValues, | ||
operationName, | ||
fieldResolver, | ||
typeResolver, | ||
} = args; | ||
const { schema, document, variableValues } = args; | ||
|
||
// If arguments are missing or incorrect, throw an error. | ||
assertValidExecutionArguments(schema, document, variableValues); | ||
|
||
// If a valid execution context cannot be created due to incorrect arguments, | ||
// a "Response" with only errors is returned. | ||
const exeContext = buildExecutionContext( | ||
schema, | ||
document, | ||
rootValue, | ||
contextValue, | ||
variableValues, | ||
operationName, | ||
fieldResolver, | ||
typeResolver, | ||
); | ||
const exeContext = buildExecutionContext(args); | ||
|
||
// Return early errors if execution context failed. | ||
if (!('schema' in exeContext)) { | ||
return { errors: exeContext }; | ||
} | ||
|
||
// Return a Promise that will eventually resolve to the data described by | ||
// The "Response" section of the GraphQL specification. | ||
// | ||
// Return data or a Promise that will eventually resolve to the data described | ||
// by the "Response" section of the GraphQL specification. | ||
|
||
// If errors are encountered while executing a GraphQL field, only that | ||
// field and its descendants will be omitted, and sibling fields will still | ||
// be executed. An execution which encounters errors will still result in a | ||
// resolved Promise. | ||
const data = executeOperation(exeContext, exeContext.operation, rootValue); | ||
const data = executeQueryOrMutationRootFields(exeContext); | ||
|
||
// Return the response. | ||
return buildResponse(exeContext, data); | ||
} | ||
|
||
|
@@ -271,15 +257,20 @@ export function assertValidExecutionArguments( | |
* @internal | ||
*/ | ||
export function buildExecutionContext( | ||
schema: GraphQLSchema, | ||
document: DocumentNode, | ||
rootValue: unknown, | ||
contextValue: unknown, | ||
rawVariableValues: Maybe<{ readonly [variable: string]: unknown }>, | ||
operationName: Maybe<string>, | ||
fieldResolver: Maybe<GraphQLFieldResolver<unknown, unknown>>, | ||
typeResolver?: Maybe<GraphQLTypeResolver<unknown, unknown>>, | ||
args: ExecutionArgs, | ||
): ReadonlyArray<GraphQLError> | ExecutionContext { | ||
const { | ||
schema, | ||
document, | ||
rootValue, | ||
contextValue, | ||
variableValues: rawVariableValues, | ||
operationName, | ||
fieldResolver, | ||
typeResolver, | ||
subscribeFieldResolver, | ||
} = args; | ||
|
||
let operation: OperationDefinitionNode | undefined; | ||
const fragments: ObjMap<FragmentDefinitionNode> = Object.create(null); | ||
for (const definition of document.definitions) { | ||
|
@@ -334,19 +325,19 @@ export function buildExecutionContext( | |
variableValues: coercedVariableValues.coerced, | ||
fieldResolver: fieldResolver ?? defaultFieldResolver, | ||
typeResolver: typeResolver ?? defaultTypeResolver, | ||
subscribeFieldResolver: subscribeFieldResolver ?? defaultFieldResolver, | ||
errors: [], | ||
}; | ||
} | ||
|
||
/** | ||
* Implements the "Executing operations" section of the spec. | ||
* Executes the root fields specified by query or mutation operation. | ||
*/ | ||
function executeOperation( | ||
function executeQueryOrMutationRootFields( | ||
exeContext: ExecutionContext, | ||
operation: OperationDefinitionNode, | ||
rootValue: unknown, | ||
): PromiseOrValue<ObjMap<unknown> | null> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Context is a necessary evil, for stuff used deeply in the call chain. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Especially, since a plan is to separate operation execution context later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Debatable, but Ok |
||
const type = getOperationRootType(exeContext.schema, operation); | ||
const { schema, operation, rootValue } = exeContext; | ||
const type = getOperationRootType(schema, operation); | ||
const fields = collectFields( | ||
exeContext.schema, | ||
exeContext.fragments, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,11 @@ import type { GraphQLFieldResolver } from '../type/definition'; | |
|
||
import { getOperationRootType } from '../utilities/getOperationRootType'; | ||
|
||
import type { ExecutionResult, ExecutionContext } from './execute'; | ||
import type { | ||
ExecutionArgs, | ||
ExecutionResult, | ||
ExecutionContext, | ||
} from './execute'; | ||
import { collectFields } from './collectFields'; | ||
import { getArgumentValues } from './values'; | ||
import { | ||
|
@@ -25,16 +29,16 @@ import { | |
} from './execute'; | ||
import { mapAsyncIterator } from './mapAsyncIterator'; | ||
|
||
export interface SubscriptionArgs { | ||
schema: GraphQLSchema; | ||
document: DocumentNode; | ||
rootValue?: unknown; | ||
contextValue?: unknown; | ||
variableValues?: Maybe<{ readonly [variable: string]: unknown }>; | ||
operationName?: Maybe<string>; | ||
fieldResolver?: Maybe<GraphQLFieldResolver<any, any>>; | ||
subscribeFieldResolver?: Maybe<GraphQLFieldResolver<any, any>>; | ||
} | ||
/** | ||
* @deprecated use ExecutionArgs instead. | ||
* | ||
* ExecutionArgs has been broadened to include all properties | ||
* within SubscriptionArgs. The SubscriptionArgs type is retained | ||
* for backwards compatibility and will be removed in the next major | ||
* version. | ||
*/ | ||
// eslint-disable-next-line @typescript-eslint/no-empty-interface | ||
export interface SubscriptionArgs extends ExecutionArgs {} | ||
|
||
/** | ||
* Implements the "Subscribe" algorithm described in the GraphQL specification. | ||
|
@@ -141,31 +145,31 @@ export async function createSourceEventStream( | |
contextValue?: unknown, | ||
variableValues?: Maybe<{ readonly [variable: string]: unknown }>, | ||
operationName?: Maybe<string>, | ||
fieldResolver?: Maybe<GraphQLFieldResolver<any, any>>, | ||
subscribeFieldResolver?: Maybe<GraphQLFieldResolver<any, any>>, | ||
): Promise<AsyncIterable<unknown> | ExecutionResult> { | ||
// If arguments are missing or incorrectly typed, this is an internal | ||
// developer mistake which should throw an early error. | ||
assertValidExecutionArguments(schema, document, variableValues); | ||
|
||
// If a valid execution context cannot be created due to incorrect arguments, | ||
// a "Response" with only errors is returned. | ||
const exeContext = buildExecutionContext( | ||
const exeContext = buildExecutionContext({ | ||
schema, | ||
document, | ||
rootValue, | ||
contextValue, | ||
variableValues, | ||
operationName, | ||
fieldResolver, | ||
); | ||
subscribeFieldResolver, | ||
}); | ||
|
||
// Return early errors if execution context failed. | ||
if (!('schema' in exeContext)) { | ||
return { errors: exeContext }; | ||
} | ||
|
||
try { | ||
const eventStream = await executeSubscription(exeContext); | ||
const eventStream = await executeSubscriptionRootField(exeContext); | ||
|
||
// Assert field returned an event stream, otherwise yield an error. | ||
if (!isAsyncIterable(eventStream)) { | ||
|
@@ -186,7 +190,7 @@ export async function createSourceEventStream( | |
} | ||
} | ||
|
||
async function executeSubscription( | ||
async function executeSubscriptionRootField( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's keep, naming for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See comment above, disagree strongly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is nothing premature about correcting an error, just because you will further improve later, this is internal code refactoring, non breaking changes, you can rererename as often as you want |
||
exeContext: ExecutionContext, | ||
): Promise<unknown> { | ||
const { schema, fragments, operation, variableValues, rootValue } = | ||
|
@@ -228,7 +232,7 @@ async function executeSubscription( | |
|
||
// Call the `subscribe()` resolver or the default resolver to produce an | ||
// AsyncIterable yielding raw payloads. | ||
const resolveFn = fieldDef.subscribe ?? exeContext.fieldResolver; | ||
const resolveFn = fieldDef.subscribe ?? exeContext.subscribeFieldResolver; | ||
const eventStream = await resolveFn(rootValue, args, contextValue, info); | ||
|
||
if (eventStream instanceof Error) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed previously want to put
subscription
here.So let's keep the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name as now is incorrect, for internal refactoring, better to just improve instead of adding todo to get optimal solution. There is a saying, “perfect is the enemy of good.”