consider changing the type-signature of parse and validate functions to return a MaybePromise

[n1ru4l]

Both parse and validate do 100% sync logic, so this request might seem obscure. Please let me try to break this down and propose why we might want to consider changing the parse and validate return types.

Today all major GraphQL.js server and frameworks built on graphql-js wrap the functions parse, validate, execute and subscribe.

execute and subscribe both already return a MaybePromise as the underlying logic can either be resolved sync or async, depending on the resolvers called or the errors occurring.

Many GraphQL servers allow overwriting the parse, validate, execute, and subscribe functions. In user-land functionality of those functions can be customized by wrapping the original graphql-js functions and then forwarding them to the server configuration as parameters.

For example, caching for parse could be simply achieved in the following way:

parse in memory cache

import { parse as originalParse } from "graphql"
import { buildCacheKey } from "./buildCacheKey"

// very cheap implementation with memory leaks :)
const cheapCache = {}

function parse(args) {
  const cacheKey = buildCacheKey(args)
  let cachedValue = cheapCache[cacheKey]
  if (cachedValue != undefined) {
    return cachedValue 
  }
  const result = originalParse(args)
  cheapCache[cacheKey] = result
  return result
}

For very big operations and GraphQL server replicas or serverless/edge worker environments, we might want to use a shared cache between our GraphQL handlers. E.g. we want to store our parsed operations or validation results within Redis instead of simply in memory.

async validate cache using redis

import { validate as originalValidate } from "graphql"
import { buildCacheKey } from "./buildCacheKey"
import { redisClient } from "./redisClient"

async function validate(args) {
  const cacheKey = buildCacheKey(args)
  let cachedValue = await redisClient.get(cacheKey)
  if (cachedValue != undefined) {
    return cachedValue 
  }
  const result = originalValidate (args)
  await redisClient.set(cacheKey, result)
  return result
}

We can easily build this function, however, if passed to a server those libraries will not expect validate to return a Promise and usually raise an error or cause other unexpected behavior.

This leads me to the following conclusion:

If the parse and validate functions return a MaybePromise instead of "only" a sync value, graphql-js enforces that server framework implementors ensure a Promise returned from parse and validate is correctly handled.

Today adopters of different server frameworks have to convince the framework maintainers individually to allow a validate or parse functions to return a Promise.

As a result, each framework comes up with a different way of hooking into those phases. An example for this is mercurius which has its own custom API for hooking into the graphql lifecycle.

With envelop, The Guild started a project that gives people a common interface for adding caching and other features to the parse, validate, execute, and subscribe functions, while preserving the original function signature in order to be compatible with ANY high-level GraphQL framework. Adopters keep asking why they can not run async logic in the pre and post-validate/parse hooks and our answer to this is that we want to have 100% API compatibility with graphql-js, which limits people from innovating the plugin eco-system (n1ru4l/envelop#497).

This led me to think about whether it might make sense to change the types of parse and validate. This would have no impact on the current implementation of the functions. It would, however, be a breaking TypeScript change and graphql-js libraries would be forced to adapt to that change (usually by just adding a await in front of validate or parse (which ib probably 99.99% of the use-cases is already happening inside an async context).

[dotansimha]

I think I'm convinced 🤔 if we'll start this change from graphql-js it might easier to align to it in other libraries. I guess that would require a major release?

[yaacovCR]

We could potentially start this approach even in a non-breaking way (at least in graphql-js) by just exporting a new type:

type GraphQLParseFn = MaybePromise<typeof parse>;

And encourage servers that allow overriding parse to change to that type signature for the parse function.

Servers could use that type even without changing the actual type signature of the parse function. Having this new type within and exported by graphql-js likely would help push the ecosystem along.

[n1ru4l]

We could potentially start this approach even in a non-breaking way (at least in graphql-js) by just exporting a new type:

type GraphQLParseFn = MaybePromise<typeof parse>;

And encourage servers that allow overriding parse to change to that type signature for the parse function.

Servers could use that type even without changing the actual type signature of the parse function. Having this new type within and exported by graphql-js likely would help push the ecosystem along.

Do you actually mean the following?

import * as gql from "graphql"
type ParseType = typeof gql.parse
type MaybPromise<T> = T | Promise<T>

type GraphQLParse = (...args: Parameters<ParseType>) => MaybPromise<ReturnType<ParseType>>

Example on TypeScript Playground

I cannot see why the following would help:

type GraphQLParseFn = MaybePromise<typeof parse>;

[yaacovCR]

I do!

[benjie]

If we add this, I'd encourage also adding parseSync and validateSync to match graphqlSync and executeSync

graphql-js/src/graphql.ts

Lines 82 to 91 in da57238

	export function graphqlSync(args: GraphQLArgs): ExecutionResult {
	const result = graphqlImpl(args);
	// Assert that the execution was synchronous.
	if (isPromise(result)) {
	throw new Error('GraphQL execution failed to complete synchronously.');
	}
	return result;
	}

graphql-js/src/execution/execute.ts

Lines 223 to 232 in da57238

	export function executeSync(args: ExecutionArgs): ExecutionResult {
	const result = execute(args);
	// Assert that the execution was synchronous.
	if (isPromise(result)) {
	throw new Error('GraphQL execution failed to complete synchronously.');
	}
	return result;
	}

There's simplicity in knowing that parse and validate are synchronous, but there's power in allowing them to be asynchronous. By adding explicit parseSync and validateSync we allow users to opt into simplicity (and opt-out of power), so that we can serve everyone's needs.

I've gone back and forth on this, but in the end I'm in favour of this change for parse. @n1ru4l's example of a parser cache is a good one. The overhead of this change would be extremely minimal (an isPromise check in a single place) so I don't see any major reason to avoid it.

For validate it's less clear, on top of caching I can think of rate-limiting and query-cost examples that may require async logic, but I would typically only perform these after the normal validation has passed (or maybe do them before even the parsing takes place...). I also wonder what making validate async would mean for the validation rules themselves - perhaps users that see an async validate would expect for validation rules to be able to be async also.