Skip to content

Allow consumers to specify a custom data type for execute #2862

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

Closed

Conversation

GreenGremlin
Copy link

This change is a follow-on of #2490, which fixed an issue where consumers of execute were forced to specify the data type generic when calling execute or when using the ExecutionResult type. Currently, ExecutionResult allows the data type generic to be overridden, but execute does not. With this change, execute is still callable without specifying the data type, but will optionally accept an override to the default data type.

With this change:

A consumer can override the default data type by passing a generic to execute.

async function executeUserQuery(): ExecutionResult<User> {
  return await execute<User>(schema, userQuery, root, context, variableValues);
}

Or, by generically using graphql-typed-document-node's TypedDocumentNode, generated by graphql-code-generator.

async function executeQuery<T extends TypedDocumentNode>(queryDoc: T): ExecutionResult<ResultOf<T>> {
  return await execute<User>(schema, userQuery, root, context, variableValues);
}

But, execute can still be called without specifying a data type. The default { [key: string]: any } is used.

await execute(schema, document, root, context, variableValues)

@GreenGremlin GreenGremlin changed the title Allow consumers to specify the data type for execute Allow consumers to specify a custom data type for execute Nov 25, 2020
@IvanGoncharov
Copy link
Member

@GreenGremlin To move this forward can you please explain why this wouldn't work:

async function executeUserQuery(): ExecutionResult<User> {
  return (await execute(schema, userQuery, root, context, variableValues)) as ExecutionResult<User>;
}

execute can't enforce type check since field resolution is fully dynamic so execute<User>(...) is the same as execute(...) as ExecutionResult<User>.
So if execute<User> is just a glorified type-cast then what it does is just provide a false sense of security.
Not a TS expert so maybe I'm missing something so please explain why as doesn't work in your case?

@GreenGremlin
Copy link
Author

@GreenGremlin To move this forward can you please explain why this wouldn't work:

async function executeUserQuery(): ExecutionResult<User> {
  return (await execute(schema, userQuery, root, context, variableValues)) as ExecutionResult<User>;
}

execute can't enforce type check since field resolution is fully dynamic so execute<User>(...) is the same as execute(...) as ExecutionResult<User>.
So if execute<User> is just a glorified type-cast then what it does is just provide a false sense of security.
Not a TS expert so maybe I'm missing something so please explain why as doesn't work in your case?

The return type can't be enforced, though with tools like graphql-codegen and graphql-typed-document-node, we can be relatively certain of its type. While casting does work, some code bases restrict its use.

Another route to go with execute would be to add an overload that takes a GraphqlTypedDocumentNode, instead of a DocumentNode. Then, we could base the return type on the document.

@IvanGoncharov
Copy link
Member

@GreenGremlin Even with GraphqlTypedDocumentNode, we can't be sure about the output.
For example, directives can change the result type e.g. experimental @stream and @defer.
That's why I think supporting it will just give you a false sense of security compared to do as which is more explicit.
That said nothing prevents you from creating a wrapper that accepts GraphqlTypedDocumentNode, calls execute and uses as to type response.

So if a question is just where to put as inside this lib or outside I'm against adding as inside lib.
That said I'm opened to discussion if someone can provide additional arguments for why it should be done inside the lib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants