Skip to content

docs: add type inference with TypedDocument for variableValues - #2560

Merged
benjie merged 5 commits into
graphile:mainfrom
Sleepful:main
Jul 22, 2025
Merged

benjie merged 5 commits into
graphile:mainfrom
Sleepful:main

Conversation

@Sleepful

@Sleepful Sleepful commented Jun 8, 2025 •

Copy link
Copy Markdown
Contributor

Description

Better type inference, I am not a fan of as any but that's what it takes for now, and allows variableValues to be typed-checked at executeDocument callsite

@github-project-automation github-project-automation Bot moved this to 🌳 Triage in V5.0.0 Jun 8, 2025
@changeset-bot

changeset-bot Bot commented Jun 8, 2025 •

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 94768ab

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

This will check for nested properties in GQL types, such as those used in patch operations, and it will report an error for additional properties in the variableValues, as it should because having additional properties not required by the query results in a GQL error.

E.g.

Object literal may only specify known properties, and 'a' does not exist in type `SomeGqlType`
@Sleepful

Sleepful commented Jun 8, 2025

Copy link
Copy Markdown
Contributor Author

Added another commit, now it properly check for nested properties in the variableValues and reports an error if there are any additional properties that should not be there!

Example:

The query, just for completeness:

  const q = graphql(`
    mutation Patch($rowId: Int!, $sliderPatch: sliderPatch!) {
      updateSliderByRowId(
        input: {sliderPatch: $sliderPatch, rowId: $rowId}
      ) {
        clientMutationId
        slider {
          enDescription
          esDescription
          id
          image
          rowId
          url
        }
      }
    }
`)

Here is where it gets better reporting, ignoring the first argument of requestContext for this example:

  const res = await executeDocument({}, q, {
    sliderPatch:
      { enDescription, esDescription, image, url, a: "" },
                                    // highlights ^ `a` and reports error!
      rowId
    })

Error reported:

Object literal may only specify known properties, and 'a' does not exist in type '{ url?: string | null | undefined; esDescription?: string | null | undefined; enDescription?: string | null | undefined; image?: string | null | undefined; }'

Necessary for proper inference of ExecutionResult
Comment on lines +223 to +228
export async function executeDocument<TData = any, TVariables = any>(
export async function executeDocument<TDoc extends TypedDocumentNode<TData, TVariables>, TData = any, TVariables = any>(
requestContext: Partial<Grafast.RequestContext>,
document: DocumentNode | TypedDocumentNode<TData, TVariables>,
variableValues?: Record<string, unknown> | null,
document: TDoc,
variableValues?: VariablesOf<typeof document>,
operationName?: string,
): Promise<ExecutionResult<TData, TVariables>> {
): Promise<ExecutionResult<ResultOf<typeof document>, TVariables>> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we need generics for TData/TVariables, maybe something like this would work:

export async function executeDocument<TDoc extends TypedDocumentNode | DocumentNode, TExtensions = ObjMap<unknown>>(
  requestContext: Partial<Grafast.RequestContext>,
  document: TDoc,
  variableValues?: VariablesOf<TDoc>,
  operationName?: string,
): Promise<ExecutionResult<ResultOf<TDoc>, TExtensions>> {

(Also the second parameter to ExecutionResult is TExtensions not TVariables - I recognize this was already incorrect.)

@benjie

benjie commented Jun 23, 2025

Copy link
Copy Markdown
Member

Hi @Sleepful, gentle nudge to see if you're interested in the above change?

@benjie benjie moved this from 🌳 Triage to 📝 Docs Improvements in V5.0.0 Jul 9, 2025
@github-project-automation github-project-automation Bot moved this from 📝 Docs Improvements to 🌱 In Progress in V5.0.0 Jul 21, 2025
@benjie
benjie merged commit b72034f into graphile:main Jul 22, 2025
@github-project-automation github-project-automation Bot moved this from 🌱 In Progress to ✅ Done in V5.0.0 Jul 22, 2025
@Sleepful

Sleepful commented Aug 8, 2025

Copy link
Copy Markdown
Contributor Author

Thanks for taking care of the diff and merging it 💙

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

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants