API for mutating documents

[jasonkuhrt]

Hey I'm asking here because I think I might just not be getting something obvious. In GraphQL the node types that represent a document appear to be read-only. How can one mutate them (say, inject a field to the root selection set) without recreating the whole document? I don't see anything Bout mutable types in the repo so maybe I'm missing an obviously solution.

export declare type DefinitionNode =
  | ExecutableDefinitionNode
  | TypeSystemDefinitionNode
  | TypeSystemExtensionNode;
export declare type ExecutableDefinitionNode =
  | OperationDefinitionNode
  | FragmentDefinitionNode;
export interface OperationDefinitionNode {
  readonly kind: Kind.OPERATION_DEFINITION;
  readonly loc?: Location;
  readonly operation: OperationTypeNode;
  readonly name?: NameNode;
  readonly variableDefinitions?: ReadonlyArray<VariableDefinitionNode>;
  readonly directives?: ReadonlyArray<DirectiveNode>;
  readonly selectionSet: SelectionSetNode;
}
declare enum OperationTypeNode {
  QUERY = 'query',
  MUTATION = 'mutation',
  SUBSCRIPTION = 'subscription',
}
export { OperationTypeNode };
export interface VariableDefinitionNode {
  readonly kind: Kind.VARIABLE_DEFINITION;
  readonly loc?: Location;
  readonly variable: VariableNode;
  readonly type: TypeNode;
  readonly defaultValue?: ConstValueNode;
  readonly directives?: ReadonlyArray<ConstDirectiveNode>;
}
export interface VariableNode {
  readonly kind: Kind.VARIABLE;
  readonly loc?: Location;
  readonly name: NameNode;
}
export interface SelectionSetNode {
  kind: Kind.SELECTION_SET;
  loc?: Location;
  selections: ReadonlyArray<SelectionNode>;
}
export declare type SelectionNode =
  | FieldNode
  | FragmentSpreadNode
  | InlineFragmentNode;

[jasonkuhrt]

So, although the types say so, it's not like the objects are actually frozen. Mutating does work at runtime:

This is a bit confusing. Are there unforeseen consequences by doing this? It's quite convenient for some use-cases though. If its inconsequential to do so, then I don't see why the types are read-only.

[yaacovCR]

It might be dangerous/unpredictable to mutate documents if they are ever reused. It would be safest to create a new document or other node with a modified attribute as necessary.

[jasonkuhrt]

My library Graffle has a TypeScript interface that produces a DocumentNode upon a request. There is a next step in that pipeline that injects __typename selection on root type fields that meet a certain criteria. In this case, copying would be wasteful. Given JS does not currently have built in performant immutable data structures, I don't think the community graphql package should force a pattern here. I think read-only assumes too much.

Making the existing types become mutable should not be a breaking change. However, it would still be a big change I imagine. Thus perhaps instead we can add to graphql package a set of Mutable node types instead, along with new functions parseToMutable etc. that output these types. IIUC there is actually zero runtime enforcement of the read-only types currently, (e.g. no Object.freeze) so parse and parseToMutable would be actually the same function in JS, until such a time that graphql got more "correct" in this regard and had its types and values align in behaviour.

wdyt?

[yaacovCR]

It seems a bit cumbersome to have two versions of the types, so I experimented a bit with some generic types:

interface WithReadOnlyProperties {
    readonly a: string;
}

type Mutable<T> = {
    -readonly [P in keyof T]: T[P];
};

type MutableVersion = Mutable<WithReadOnlyProperties>;

function toMutable<T>(arg: T): Mutable<T> {
    return arg;
}

function toImmutable<T>(arg: Mutable<T>): T {
    return arg;
}

const a: WithReadOnlyProperties = { a: 'a' };

a.a = 'aa'; // Cannot assign to 'a' because it is a read-only property.

(a as Mutable<WithReadOnlyProperties>).a = 'aa'; // Success!

const mutableA = toMutable(a);

mutableA.a = 'aa';

const immutableA = toImmutable(mutableA);

The above kind of looks promising for your purposes -- I think. You have the conversion functions you requested, and for someone who is more of a purist (?), there is the explicit cast instead which has zero runtime cost.

But if that's enough to satisfy your needs, I wonder if we really need to add it to graphql. Seems like it might be able to live in user land?

Within graphql-js, AST nodes generally (although not documents per se) have to stay readonly because we expose them within the GraphQLResolveInfo and if we let them be modified during execution, it could lead to unstable behavior.

Those are some initial thoughts! I do wonder what the other clients do. Perhaps they just take the performance hit? Maybe they want immutable structures because they have caching layers within the pipeline?

[jasonkuhrt]

Thanks for looking at this @yaacovCR!

But if that's enough to satisfy your needs, I wonder if we really need to add it to graphql. Seems like it might be able to live in user land?

Given the graphql package doesn't have functional logic that depends on the immutable nature of the document nodes (outside of the resolver function you mentioned), and does not enforce it (as mentioned e.g. Object.freeze) then I think this can be solved outside of the core package. In other words, if the only thing at stake is TypeScript types and nothing at runtime is actually implicated, then yeah, maybe with a bit of careful internal cheating (casting) we can have a parallel mutable document type in addition to the immutable core one.

It might be useful to document for library authors somewhere a brief guide with regards to mutating documents. Where it's definitely going to cause problems versus everywhere else where it will not.