Conversation
There are a number of reasons why enums are bad, but the biggest problem — the one affects us most — is that enums behave differently than the rest of TypeScript: while TypeScript is usually a **structural type system**, enums break the mold and are treated as **nominal types**. This creates unintentional type errors. For instance, if one package has a function which takes an argument of type `KnownCaipNamespace` from `@metamask/utils` 11.1.0, and another package attempts to call that function with a member of `KnownCaipNamespace` from `@metamask/utils` 11.0.0, then a type error would be produced, even if the enums across both versions have the same exact contents. The second package would need to bump to 11.1.0 to fix the error. There are other strange and undesirable "features" of enums that we do use. These are detailed here: MetaMask/eslint-config#417 To prevent these kinds of problems from occurring in the future, this commit adds a lint rule to ban the use of enums, and updates all instances of enums to use object literals and types instead.
| export type KnownCaipNamespace = | ||
| (typeof KnownCaipNamespace)[keyof typeof KnownCaipNamespace]; |
There was a problem hiding this comment.
Interesting, at first glance it seems like this would create a circular type reference, but is it actually referring to the object KnownCaipNamespace here?
There was a problem hiding this comment.
Yes, that's right. If this had been written from scratch then I would have used KNOWN_CAIP_NAMESPACES for the variable and KnownCaipNamespace for the type. I wanted to minimize breaking changes here, but if you think it's going to be too confusing I can change it. What do you think?
There was a problem hiding this comment.
I think this is fine. Wondering if it makes sense to add a simple helper for this though, maybe it makes it a little bit easier to understand?
| export type KnownCaipNamespace = | |
| (typeof KnownCaipNamespace)[keyof typeof KnownCaipNamespace]; | |
| export type ObjectToUnion<Type> = Type[keyof Type]; | |
| export type KnownCaipNamespace = ObjectToUnion<typeof KnownCaipNamespace>; |
There was a problem hiding this comment.
IMO that's neat. Value and type have matching names and they should not conflict. But I guess that's a personal opinion 😁
| rules: { | ||
| 'no-restricted-syntax': [ | ||
| 'error', | ||
| { | ||
| selector: 'TSEnumDeclaration', | ||
| message: | ||
| "Don't use enums. There are a number of reasons why they are problematic, but the most important is that TypeScript treats them nominally, not structurally, and this can cause unexpected breaking changes. Instead, use an object + type, an array + type, or just a type. Learn more here: https://github.com/MetaMask/eslint-config/issues/417", | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
We should add this to the shared ESLint configs at some point.
There was a problem hiding this comment.
I see you created an issue for this already, so disregard this comment. 😅
There was a problem hiding this comment.
We talked about enabling this in accounts too, but indeed if that's enforced by the shared eslint, that's even better 🚀
3 participants
There are a number of reasons why enums are bad, but the biggest problem — the one that affects us most — is that enums behave differently than the rest of TypeScript: while TypeScript is usually a structural type system, enums break the mold and are treated as nominal types.
This creates unintentional type errors. For instance, if one package has a function which takes an argument of type
KnownCaipNamespacefrom@metamask/utils11.1.0, and another package attempts to call that function with a member ofKnownCaipNamespacefrom@metamask/utils11.0.0, then a type error would be produced, even if the enums across both versions have the same exact contents. The second package would need to bump to 11.1.0 to fix the error.There are other strange and undesirable "features" of enums that we do use. These are detailed here:
MetaMask/eslint-config#417
To prevent these kinds of problems from occurring in the future, this commit adds a lint rule to ban the use of enums, and updates all instances of enums to use object literals and types instead.
Note
Medium Risk
Introduces breaking type changes to exported constants that may require downstream TypeScript updates, though runtime access patterns like
Duration.Secondshould remain compatible.Overview
This PR bans TypeScript
enumusage via an ESLintno-restricted-syntaxrule forTSEnumDeclarationto prevent future enum additions.It replaces the exported enums
KnownCaipNamespace,JsonSize, andDurationwithas constobject literals plus derived union types, and updates the type tests/usages accordingly (e.g.,Object.values(Duration)now yields typed numeric values). The changelog is updated to call out the breaking type-level impact for consumers relying on the enum types in signatures.Reviewed by Cursor Bugbot for commit f112624. Bugbot is set up for automated code reviews on this repo. Configure here.