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

Support TypedDocumentNode in graphql contracts #773

Closed
longility opened this issue Jun 7, 2021 · 3 comments · Fixed by #774
Closed

Support TypedDocumentNode in graphql contracts #773

longility opened this issue Jun 7, 2021 · 3 comments · Fixed by #774

Comments

@longility
Copy link
Contributor

longility commented Jun 7, 2021

Is your feature request related to a problem? Please describe.
I would like consistent usage of TypedDocumentNode approach for graphql operations and mocks (msw) to minimize cognitive load, coupling, and explicit types needing to be added.

Describe the solution you'd like
I would like to be able to do this

import { GetRecentActivitiesDocument } from "graphql-operations";
import { graphql } from "msw";

export const getRecentActivitiesHandler = graphql.query(
  GetRecentActivitiesDocument, // generated type that is of type `TypedDocumentNode<,>` from '@graphql-typed-document-node/core'
  (req, res, ctx) => {
    // removed for brevity
  }
);

instead of this

import {
  GetRecentActivitiesQuery,
  GetRecentActivitiesQueryVariables,
} from "graphql-operations";
import { graphql } from "msw";

export const getRecentActivitiesHandler = graphql.query<
  GetRecentActivitiesQuery,
  GetRecentActivitiesQueryVariables
>("GetRecentActivities", (req, res, ctx) => {
  // removed for brevity
});

Describe alternatives you've considered
I did not consider other alternatives as it seems straightforward.

Additional context
If you haven't heard of TypedDocumentNode, here are some references

import { GetRecentActivitiesDocument } from "graphql-operations";
// ...
const { data, refetch } = useQuery(GetRecentActivitiesDocument, {
    variables: { limit: 10 },
  }); // apollo client usage of `TypedDocumentNode`

Questions

  1. Is this something y'all would be supporting in the codebase?
  2. If so, is this easy to do where y'all can do in a short amount of time or possibly guide me on how y'all want to approach so I can do and submit a PR?
@kettanaito
Copy link
Member

kettanaito commented Jun 7, 2021

Hey, @longility. Thanks for suggesting this.

It looks like an interesting idea to explore, let's elaborate on it. I'm not familiar with TypedDocumentNode, going through the articles you've posted above. I'll include my observations and concerns below.

So TypedDocumentNode is a pre-compiled DocumentNode derived from your operation + schema. A really good example is found in this sandbox. Since it's a DocumentNode, it contains an operation definition, including operation kind and type—two factors necessary to determine if a request matches a handler.

From what I can see at the moment, supporting TypedDocumentNode should be rather trivial. Will list the high-level necessary steps for this support below.

  1. Support the TypedDocumentNode as the input type in the GraphQL handler:

export type GraphQLHandlerNameSelector = RegExp | string

  1. Parse the operationName if it's TypedDocumentNode to extract operation type and name store in the handler's info object:

super({
info: {
header,
operationType,
operationName,
},
ctx: graphqlContext,
resolver,
})

We can use the same parseQuery utility used for parsing actual requests, as we're doing the same thing with the typed document node:

function parseQuery(query: string): ParsedGraphQLQuery | Error {
try {
const ast = parse(query)
const operationDef = ast.definitions.find((def) => {
return def.kind === 'OperationDefinition'
}) as OperationDefinitionNode
return {
operationType: operationDef?.operation,
operationName: operationDef?.name?.value,
}
} catch (error) {
return error
}
}

// In the GraphQLHandler constructor:
const { operationName: parsedOperationName } = parseQuery(operationName)
  1. Account for incompatible TypedDocumentNode operation kind. Prevent passing a mutation document node to the graphql.query handler and vice versa. Should be done in the GraphQLHandler constructor (the earliest available point):
constructor(operationType, operationName, ...rest) {
  if (operationName instanceof DocumentNode) {
    const { operationType: parsedOperationType } = parseQuery(operationName)
    if (parsedOperationType !== operationType) {
      throw new Error('Invalid DocumentNode provided: expected "${operationType}", got "${parsedOperationType}"')
    }
  }
}

This logic belongs in the GraphQLHandler constructor, as it's a user input validation and not a request predicate (no request present at this point, and is request-irrelevant).

Once we've set the handler's info correctly, the parsed operation name is then used in the predicate and other parts of the handler, no changes should be necessary. A few integration tests remain, and the feature looks good.

Please, @longility, would you be interested in leading this effort?

@longility
Copy link
Contributor Author

Yep, I heard about TypedDocumentNode this past week so I'm not an expert. It is just something that seems I would love to go towards.

@kettanaito thanks for the quick analysis and guidance. If you are okay with me taking a couple of weeks as I can't dedicate my time on this, then I'm okay leading this effort.

@kettanaito
Copy link
Member

For sure, there's no rush. Thank you for your help, @longility!

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

Successfully merging a pull request may close this issue.

2 participants