Skip to content
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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 32 additions & 0 deletions src/execution/__tests__/subscribe-test.ts
Expand Up @@ -256,6 +256,38 @@ describe('Subscription Initialization Phase', () => {
await subscription.return();
});

it('uses a custom default subscribeFieldResolver', async () => {
const schema = new GraphQLSchema({
query: DummyQueryType,
subscription: new GraphQLObjectType({
name: 'Subscription',
fields: {
foo: { type: GraphQLString },
},
}),
});

async function* fooGenerator() {
yield { foo: 'FooValue' };
}

const subscription = await subscribe({
schema,
document: parse('subscription { foo }'),
rootValue: { customFoo: fooGenerator },
subscribeFieldResolver: (root) => root.customFoo(),
});
invariant(isAsyncIterable(subscription));

expect(await subscription.next()).to.deep.equal({
done: false,
value: { data: { foo: 'FooValue' } },
});

// Close subscription
await subscription.return();
});

it('should only resolve the first field of invalid multi-field', async () => {
async function* fooGenerator() {
yield { foo: 'FooValue' };
Expand Down
65 changes: 28 additions & 37 deletions src/execution/execute.ts
Expand Up @@ -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>;
}

Expand Down Expand Up @@ -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>>;
}

/**
Expand All @@ -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);
}

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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(
Copy link
Member

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.

Copy link
Contributor Author

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.”

exeContext: ExecutionContext,
operation: OperationDefinitionNode,
rootValue: unknown,
): PromiseOrValue<ObjMap<unknown> | null> {
Copy link
Member

Choose a reason for hiding this comment

The 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.
If arguments are used directly we should pass them directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially, since a plan is to separate operation execution context later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
Expand Down
40 changes: 22 additions & 18 deletions src/execution/subscribe.ts
Expand Up @@ -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 {
Expand All @@ -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.
Expand Down Expand Up @@ -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)) {
Expand All @@ -186,7 +190,7 @@ export async function createSourceEventStream(
}
}

async function executeSubscription(
async function executeSubscriptionRootField(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep, naming for now.
It's better to refactor them to return proper response once we separate operation execution context.
We can add TODO or FIXME to make it clearer.
Just don't want to rename stuff prematurly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment above, disagree strongly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 } =
Expand Down Expand Up @@ -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) {
Expand Down