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

Add Data and Variable types to executer operation #6384

Closed
wants to merge 1 commit into from
Closed

Add Data and Variable types to executer operation #6384

wants to merge 1 commit into from

Conversation

jacksenior
Copy link

@jacksenior jacksenior commented May 5, 2022

Previously created this PR #6311 I don't seem to have the power to decline my old PR so when someone gets to this please decline it to avoid confusion
Was asked to move this to the version-4 branch and use a generic for GraphQLResponse rather than omit

Another comment was to use TypedDocumentNode I have no experience with this so can't quickly update the code to make use of this.

Reasoning
When using apollo-server-express to fulfil a rest request internally we are using the executeOperation to avoid HTTP and we want it strongly typed like our client side queries.
Rather than wrapping in a function to strictly type the variables and response of the query I've extended the function type to match how useQuery behaves in apollo-client.

Example usage

const gqlResponse = await apolloServer.executeOperation<
      EventAllQuery,
      EventAllQueryVariables
    >(
      {
        query: gql`
          query EventAll($eventId: String!) {
            event(eventId: $eventId) {
              ... on AmericanFootballEvent {
                id
                name
                eventStatus {
                  description
                }
              }
            }
          }
        `,
        variables: {
          eventId: req.params.event_id,
        },
      },
      { req, res },
    )

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 5, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 94bc3e2:

Sandbox Source
Apollo Server Typescript Configuration
Apollo Server Configuration

// sendErrorResponse in requestPipeline.ts.
// By leaving this open as the wider object type it fulfills what a user can pass
// in which is an extension of Record<string, any>.
type GraphQLResponseData = Record<string | number | symbol, any>;
Copy link
Author

Choose a reason for hiding this comment

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

This can be changed to Record<string, any> as outlined in the comment by casting further into the call stack or at the end of the call stack.

I prefer not to cast so this works for the use case happy to update if you feel otherwise

@trevor-scheer trevor-scheer added this to the Release 4.0 milestone Jun 8, 2022
@trevor-scheer
Copy link
Member

Thanks for reopening against v4 @jacksenior. I've added this to the v4 milestone, it'll be something we look at probably post-alpha. I like the idea!

@glasser
Copy link
Member

glasser commented Aug 16, 2022

I think this is a good idea and we should get it into AS4. We may want to make it work with TypedDocumentNode (and/or graphql-js' own somewhat unloved TypedQueryDocumentNode) as well. However, it involves changing the types of GraphQLResponse, and so will the work to make defer/stream work as part of AS4, so it's probably best to evaluate this after implementing incremental delivery (defer/stream).

glasser added a commit that referenced this pull request Sep 27, 2022
This lets you manually specify the these types when calling the method;
typically you generate these types with a codegen tool like
graphql-code-generator. This matches a common pattern used in client
development.

This doesn't let you specify the types for incremental delivery
responses; we could potentially add that later.

This also lets you use TypedQueryDocumentNode (from graphql-js) as the
`query`; if you do so, we can infer the two generic types automatically.
(This type is less popular than the earlier TypedDocumentNode from
`@graphql-typed-document-node/core` and we should probably support the
latter as well; in the interest of reducing dependencies we don't yet,
but we can extend to support that later.)

Adapted from an original PR #6384 by @jacksenior.
@glasser
Copy link
Member

glasser commented Sep 27, 2022

I extended this into #6960. Thanks!

@glasser glasser closed this Sep 27, 2022
glasser added a commit that referenced this pull request Sep 27, 2022
This lets you manually specify the these types when calling the method;
typically you generate these types with a codegen tool like
graphql-code-generator. This matches a common pattern used in client
development.

This doesn't let you specify the types for incremental delivery
responses; we could potentially add that later.

This also lets you use TypedQueryDocumentNode (from graphql-js) as the
`query`; if you do so, we can infer the two generic types automatically.
(This type is less popular than the earlier TypedDocumentNode from
`@graphql-typed-document-node/core` and we should probably support the
latter as well; in the interest of reducing dependencies we don't yet,
but we can extend to support that later.)

Adapted from an original PR #6384 by @jacksenior.
glasser added a commit that referenced this pull request Sep 27, 2022
This lets you manually specify the these types when calling the method;
typically you generate these types with a codegen tool like
graphql-code-generator. This matches a common pattern used in client
development.

This doesn't let you specify the types for incremental delivery
responses; we could potentially add that later.

This also lets you use TypedQueryDocumentNode (from graphql-js) as the
`query`; if you do so, we can infer the two generic types automatically.
(This type is less popular than the earlier TypedDocumentNode from
`@graphql-typed-document-node/core` and we should probably support the
latter as well; in the interest of reducing dependencies we don't yet,
but we can extend to support that later.)

Adapted from an original PR #6384 by @jacksenior.
glasser added a commit that referenced this pull request Sep 27, 2022
This lets you manually specify the these types when calling the method;
typically you generate these types with a codegen tool like
graphql-code-generator. This matches a common pattern used in client
development.

This doesn't let you specify the types for incremental delivery
responses; we could potentially add that later.

This also lets you use TypedQueryDocumentNode (from graphql-js) as the
`query`; if you do so, we can infer the two generic types automatically.
(This type is less popular than the earlier TypedDocumentNode from
`@graphql-typed-document-node/core` and we should probably support the
latter as well; in the interest of reducing dependencies we don't yet,
but we can extend to support that later.)

Adapted from an original PR #6384 by @jacksenior.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants