Upgrade issues with TypeScript enums and nominal typing

[brainkim]

Hullo! I’m upgrading some Apollo projects to GraphQL 16 and running into slight difficulties with enums. The context is that we have utilities which create or modify AST nodes:

diff --git a/src/utilities/graphql/transform.ts b/src/utilities/graphql/transform.ts
index 2a5128d05..80db6d202 100644
--- a/src/utilities/graphql/transform.ts
+++ b/src/utilities/graphql/transform.ts
@@ -2,6 +2,7 @@ import { invariant } from '../globals';
 
 import {
   DocumentNode,
+  Kind,
   SelectionNode,
   SelectionSetNode,
   OperationDefinitionNode,
@@ -53,9 +54,9 @@ export type RemoveVariableDefinitionConfig = RemoveNodeConfig<
 >;
 
 const TYPENAME_FIELD: FieldNode = {
-  kind: 'Field',
+  kind: 'Field' as Kind.FIELD,
   name: {
-    kind: 'Name',
+    kind: 'Name' as Kind.NAME,
     value: '__typename',
   },
 };

The code is semantically sound, but TypeScript requires the assertion because you must reference enums directly wherever they are required (nominal typing).

enum Foo {
  a = "A",
  b = "B",
}

// This is a type error because we’re not referencing the enum directly.
const foo: Foo = "A";
// This is fine
const foo1: Foo = Foo.a;

The disadvantages of this change in GraphQL 16 are:

1. We have to add runtime dependencies on the files which contain the enums.
2. It’s slightly tricky to support ranges of graphql-js versions.

As a specific example of an enum-based difficulty, OperationTypeNode is an enum in GraphQL 16, but it’s not exported as a value in GraphQL 15, so there is no way to reference the enum value across 15 and 16, and you get type errors if you assign strings like "query" or "mutation" to the enum.

I think the above two concerns are reason enough to switch to using string unions, which are structurally typed, and don’t require direct reference. We can make this a non-breaking change by exporting a namespace instead of an enum, and shadowing the identifier on the type-level with a string union.

export type Kind = "Name" | "Document" /* ... */;
export namespace Kind {
  export const NAME = 'Name';
  export const DOCUMENT = 'Document';
  /* ... */
}

Thanks for reading. Let me know your thoughts!

[saihaj]

Why not use the enum directly?

 const TYPENAME_FIELD: FieldNode = {
-  kind: 'Field',
+  kind: Kind.FIELD,
   name: {
-    kind: 'Name',
+    kind: Kind.NAME,
     value: '__typename',
   },
 };

Let me know your thoughts!

Exporting namespaces and string unions imo isn't very clean.

[brainkim]

@saihaj

Why not use the enum directly?

I’m not sure I want to import a file from graphql-js just to access the enum value. And in the case of OperationTypeNode, the enum does not exist in v15.

Exporting namespaces and string unions imo isn't very clean.

I agree it could be a pain to deal with and write. I still think the nominal typing part about enums is tricky. Any other solution which preserves the structural typing behavior and also allows for dot notation access would be acceptable to me.

[IvanGoncharov]

@brainkim Practically speaking is it a blocker for adding v16 support to apollo-client?
I will keep this issue open anyway just want to know how urgent it?

I’m not sure I want to import a file from graphql-js just to access the enum value. And in the case of OperationTypeNode, the enum does not exist in v15.

It looks like you are using visit in Apollo client:
https://github.com/apollographql/apollo-client/blob/8716cbcaaa5f633768f2e44055921caa7858fcf9/src/utilities/graphql/transform.ts#L16
If so you already bundling entire Kind, see:

graphql-js/src/language/visitor.ts

Lines 185 to 187 in 30b4469

	for (const kind of Object.values(Kind)) {
	enterLeaveMap.set(kind, getEnterLeaveForKind(visitor, kind));
	}

It’s slightly tricky to support ranges of graphql-js versions.

If you have any idea on how to address this happy to implement it in previous versions.

We have to add runtime dependencies on the files which contain the enums.

Const enums are ideal for this case however we can't do Object.values on them.
Ideally, TS would have a mechanism that allows us to mark enum as safe to inline but keep the runtime part for the case when you actually need it.

[benjie]

Could we do something like this, I wonder?

export const Kind = {
  OBJECT: 'ObjectType' as const,
  OBJECT_FIELD: 'ObjectField' as const
};
export type Kind = typeof Kind[keyof typeof Kind];

const a: Kind = Kind.OBJECT;
const b: Kind = 'ObjectType';

export interface ASTNode {
  kind: typeof Kind.OBJECT
}

https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBAYwgOwM7wNIEtkBM4C8cA3gFBxwDyAQgFICiAwgCoBccA5JQEYBWwCGMwCeYYBzgBDVIhToANOSp0mzAPoAxAJL0AMgBF2XPgJgaswADa4J02WhikAvgG5SoSLDgxRwONjxCb18IADN-HFwAbQBrYGEw4LFEgNwAXTdSJAcpdlSg1IA6GgYWN2z0OG48yKDjfkERMQ5Mj2h4HBhgKFDJBD8AQQBlZgA5CFw-MgoYyPYfZPCiktVnIA

[yaacovCR]

Wow, that is neat!

[brainkim]

@IvanGoncharov It’s not a blocker! Thankfully with TypeScript there are workarounds. Just surfacing the struggle in case others having it.

@benjie keyof typeof is a new one to me! That’s fantastic.

You can also use the const on the object literal, and freeze the Object to prevent unwanted mutations too.

export const Kind = Object.freeze({
  OBJECT: 'ObjectType',
  OBJECT_FIELD: 'ObjectField',
} as const);
export type Kind = typeof Kind[keyof typeof Kind];

const a: Kind = Kind.OBJECT;
const b: Kind = 'ObjectType';

export interface ASTNode {
  kind: typeof Kind.OBJECT
}

[brainkim]

Another detail is that we’re using typeof Enum in various places (https://github.com/graphql/graphql-js/blob/main/src/language/directiveLocation.ts#L33), and it doesn’t actually seem to work as expected.

enum Foo {
    a = 1,
    b = 2,
}

type FooLegacy = typeof Foo;

const foo: Foo = Foo.a;

// this errors
const foo1: FooLegacy = Foo.a;

https://www.typescriptlang.org/play?ssl=11&ssc=31&pln=1&pc=1#code/KYOwrgtgBAYg9nKBvAUFdUCGUC8UCMANGhgEa5QBMxAviigC4CeADsLAgDLADmmAxkwrM2cAGYc4Abnr84IAM4MoYhAC5JFeHAB0mGSgD0hqAwAWASwVRgAJ1txbClHMXLVcfBu3c+grQh6UkA

[yaacovCR]

Tracking in newer issue #4253

[JoviDeCroock]

These are two distinct issues, let's leave this open.

[JoviDeCroock]

This will be solved in v17 after #4254