Inconsistencies in formatted types

[Cito]

There are a couple of Formatted...Result types that don't seem to be used anywhere. Therefore, they are also not checked for consistency.

For instance, there is a FormattedCompletedResult, but not a FormattedPendingResult, and the definition of FormattedCompletedResult seems to be wrong, it should have an "id" instead of "path" and "label".

I can contribute a fix, but it's unclear to me what purpose these types have if they are not used or referenced anywhere.

Particularly, should FormattedCompletedResult be changed or simply removed and replaced with CompletedResult like PendingResult? Or should the Formatted... be aliases if they are the same as the unformatted types?

[yaacovCR]

Hi @Cito -- sorry for the late response.

We included the extra types to provide an incremental parallel to the currently exported FormattedExecutionResult.

Why does graphql-js have that type?

I am not 100% on the history, but the usefulness seems to be that in production servers, the client will receive a JSONified and sanitized version of GraphQL error objects rather than the original object of the GraphQLError class. That JSONified object has a constraining interface spelled out by the spec, which in graphql-js corresponds to GraphQLFormattedError interface. So in my understanding, if we were starting greenfield, we might better call GraphQLFormattedError => IGraphQLError or GraphQLErrorInterface.

Then, GraphQLFormattedError bleeds into other types, like FormattedExecutionResult which would have to be used instead of ExecutionResult if we wanted the formatted errors instead of the ones actually produced by the execution engine.

So, in short:

• We modelled the incremental delivery types around FormattedExecutionResult, trying to create all the parallel types.
• We don't need a FormattedPendingResult, because it does not contain any errors, and so it does not need a parallel formatted type.
• We should have touched up FormattedCompletedResult to the new response format, though, good catch!

[benjie]

Well said, @yaacovCR; the only point I'd challenge is that formatted types should only reference other formatted types and simple scalars: the fact that PendingResult is the same both formatted and unformatted currently doesn't mean that it shouldn't have its own type, we might want to add details to it in future where it differs (and if people refer to the wrong type it may break their application's build when we do so). As you say, essentially the "formatted" result is the "over the wire" result that a client would receive - e.g. errors are no longer instanceof GraphQLError but just plain data.

[yaacovCR]

formatted types should only reference other formatted types and simple scalars: the fact that PendingResult is the same both formatted and unformatted currently doesn't mean that it shouldn't have its own type, we might want to add details to it in future where it differs

Hmmmm. That's food for thought.

Overall, I would be hoping for a solution where we somehow remove the dichotomy between the "server" and "wire" formats.

Something perhaps more like

// rename FormattedGraphQLError, preserving the identifier for backwards compatibility only
type SpecifiedGraphQLError = FormattedGraphQLError 

class GraphQLError implements SpecifiedGraphQLError {
   ...
}

interface ExecutionResult<TError extends SpecifiedGraphQLError> {
  data?: ObjMap<unknown>;
  errors?: ReadonlyArray<TError>;
}

[benjie]

That seems more complex to me; I think what we have right now is quite a clean separation between runtime values and actual GraphQL specified data:

The errors entry in the response is a non-empty list of errors raised during the request, where each error is a map of data described by the error result format below.
https://spec.graphql.org/draft/#sec-Errors

i.e. the FormattedGraphQLError is a map, which is a simple key-value object.

[yaacovCR]

Let me tweak a bit:

class GraphQLError implements GraphQLFormattedError {   //   <<= Note that we added the implements but, which is already true
   ...
}

interface ExecutionResultShape<TError extends GraphQLFormattedError> {
  data?: ObjMap<unknown>;
  errors?: ReadonlyArray<TError>;
}

Then we could have:

type ExecutionResult = ExecutionResultShape<GraphQLError>
type FormattedExecutionResult = ExecutionResultShape<GraphQLFormattedError>

The generic type here actually documents the relationship between the runtime result of execute() and the wire/strict spec type so that one can instantly see that (so far) they differ only in how errors are represented.

Just a thought!

Out of curiosity, do you have anything at this point in mind in terms of portions of the return result from execute() that might differ between the runtime result and the wire/strict spec format?

[benjie]

You mean things like errors having a .originalError property, or something else?

[yaacovCR]

I mean something else that would motivate

the fact that PendingResult is the same both formatted and unformatted currently doesn't mean that it shouldn't have its own type, we might want to add details to it in future where it differs

Considering a pending result does not have associated errors