avoid using enums in favor of frozen object literals

[brainkim]

Implements the suggestion by @benjie (#3356 (comment))

[IvanGoncharov]

@brainkim This is basically a revert of #3317 but with value and type sharing the same name.
After looking into it more I see that TS enums are broken.

That said, this is a breaking change so we need to release v17 for it.
So I suggest waiting a week and seeing if any other issue will come up that requires breaking release in order to be fixed.
Also please test this solution and be sure it fully meets your requirements since it would be very painful to change it after this PR is merged.

[yaacovCR]

Why is it breaking if essentially relaxes type more than the auto type included within Enums?

[brainkim]

So the only breaking part of this change is that there will no longer be type-level emits of the enum values (type: OperationTypeNode.QUERY -> type: typeof OperationTypeNode.QUERY). But this is easy to notice, easy to fix, and some of the enum types, specifically the legacy aliases, are slightly broken anyways, so I think there’s a way to sell this as a patch change.

[benjie]

I'd sell it as a TypeScript fix for compatibility with v15 (assuming it improves compatibility with v15).

[brainkim]

@IvanGoncharov Let me know what sort of tests you’re expecting if you get a chance!

[IvanGoncharov]

Let me know what sort of tests you’re expecting if you get a chance!

@brainkim No additional tests are needed, but please fix prettier and other linters.

[IvanGoncharov]

Why is it breaking if essentially relaxes type more than the auto type included within Enums?

@yaacovCR It's breaking as Brian showed, also it breaking for a number of other reasons, e.g. enum merging, keyof Kind is not working, etc.

I think we should be truthful to the community and just release a new major version and not try to hide this change under the rug.

I also think this gives the opportunity to do a proper fix and not hack.
Having the same name for two exported things is bad, couple reasons from the top of my head:

• Tooltips in IDE would be confusing
• We will have problems with generated docs (the plan was to require JSDoc comment on everything that is exported).
• We need to have ESLint disable comment every time we define an enum
• It's super weird that typeof Kind and Kind are totally different types.

TS enums are bad but what leads us to a hacky and non-obvious solution that tries to imitate how should work in the first place.
Instead, I recognize that #3317 was a bad idea and we just need to partly revert it and have Kind (as const literal) and KindEnum (as type).
That makes the solution non-confusing and easier to understand and document.

[yaacovCR]

Hi Ivan! So you are advocating for a partial reversion of #3317 in v16? And the full change here for v17?

[brainkim]

@IvanGoncharov It’s technically a breaking change, but it’s unlikely to affect people according to the analysis above. It’s your call, but if it’s going in a major release, I’d like to know the timeline for said release so I can throw a bunch of anys into Apollo Client to avoid having to deal with this 😇

[benjie]

I think it would definitely be worth fully evaluating the differences before committing. I have to agree that TypeScript enums are broken, and that Kind and typeof Kind differing is definitely undesirable (and liable to future breakage) - having thought about it more I'd probably go with KindMap (or similar) for the lookup object, and Kind for the actual kind (a string):

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

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

export interface ASTNode {
  kind: typeof KindMap.OBJECT
}

https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBAYwgOwM7wNIEtkBMCyAhmHALxwDeAUHHAPIBCAUgKIDCAKgFxwDkdAIwBWwBDA4BPMMF5xCqRCnQAaGvWbsOAfQBiASRYAZACI9+w0TB1ZgAG1yz5itDCoBfANxVQkWHBhSwHDYeGT+gRAAZsE4BMQA2gDWwBJR4dJpIXFgALpeVEgucjxZYVlEYAB0jKycXoXocAIlsWHmImKS0rz5PtDwODDAUJGECEEAggDKHAByELhB1LSJsTwBGdHlxNUanO5AA

[brainkim]

Another data point from @ferm10n graphql/vscode-graphql#335 (comment)

[m1heng]

🤔 Just realized that if this merged, apollographql/federation#1345 some of my current work maybe need revert too?

[yaacovCR]

@brainkim main is now unstable, i.e. representing the next v17 alpha.

Do you have any further thoughts about the best path forward, if anything in your estimation has changed, etc, etc?

Would you be able to rebase this PR?

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: brainkim / name: Brian Kim (45846f4)

[netlify]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	45846f4
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/62ebc21592cf2e0008d2f036
😎 Deploy Preview	https://deploy-preview-3360--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.