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.

Sign up for GitHub

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

Jump to bottom

Properly name the root field execution functions #3294

Closed
wants to merge 2 commits into from
Closed

Properly name the root field execution functions #3294

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0b2bfb8
Deprecate SubscriptionArgs and broaden ExecutionArgs (#3295)
yaacovCR Oct 10, 2021
fdf3e33
Properly name the root field execution functions
yaacovCR Oct 7, 2021
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
21 changes: 11 additions & 10 deletions src/execution/execute.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ export interface ExecutionArgs {
* a GraphQLError will be thrown immediately explaining the invalid input.
*/
export function execute(args: ExecutionArgs): PromiseOrValue<ExecutionResult> {
const { schema, document, variableValues, rootValue } = args;
const { schema, document, variableValues } = args;

// If arguments are missing or incorrect, throw an error.
assertValidExecutionArguments(schema, document, variableValues);
Expand All @@ -180,14 +180,16 @@ export function execute(args: ExecutionArgs): PromiseOrValue<ExecutionResult> {
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 @@ -329,14 +331,13 @@ export function buildExecutionContext(
}

/**
* 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
4 changes: 2 additions & 2 deletions src/execution/subscribe.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ export async function createSourceEventStream(
}

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 @@ -190,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