From 11c8b7ca82f27debb7d7de82c4c519089f699fa3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=9C=A8=20Audrey=20=E2=9C=A8?= Date: Fri, 30 May 2025 12:37:47 -0400 Subject: [PATCH 01/13] introduce ADR-0025: deprecate typescript enums --- custom-words.txt | 1 + .../adr/0025-ts-deprecate-enums.md | 88 ++++++++++++++++++ docs/contributing/code-style/enums.md | 89 ++++++++++++++++--- 3 files changed, 165 insertions(+), 13 deletions(-) create mode 100644 docs/architecture/adr/0025-ts-deprecate-enums.md diff --git a/custom-words.txt b/custom-words.txt index ab723341a..000141948 100644 --- a/custom-words.txt +++ b/custom-words.txt @@ -86,6 +86,7 @@ Yellowpages Yubico YubiKey YubiKeys +typesafe # Forbidden words !auto-fill diff --git a/docs/architecture/adr/0025-ts-deprecate-enums.md b/docs/architecture/adr/0025-ts-deprecate-enums.md new file mode 100644 index 000000000..7f82a2a8f --- /dev/null +++ b/docs/architecture/adr/0025-ts-deprecate-enums.md @@ -0,0 +1,88 @@ +--- +adr: "0025" +status: Proposed +date: 2025-05-30 +tags: [clients, typescript] +--- + +# 0025 - Deprecate TypeScript Enum Types + + + +## Context and Problem Statement + +TypeScript experts have long discouraged the use of enums. There are a variety of cited reasons. +Among them: + +- Enums that are not `const` substantially increase output size. +- Numeric enums implicitly cast from `number` to the enum type, which allows invalid enum values to + be transmitted to functions. +- String enums are named types that require a member is used, which is inconsistent with the + behavior of numeric enums. + +These inconsistencies cause increased complexity from guard statements in the best of cases. In the +worst cases, their limitations may be unknown, and thus unguarded. Reducing or eliminating + +### Detection + +Typescript deprecation can be linted using a fairly short ESlint plugin. The code has +[already been contributed to main](no-enum-lint) as a suggestion. The same PR adds `FIXME` comments +for each team to address. + +### Replacement code + +In most cases, enums are unnecessary. A readonly (`as const`) object coupled with a type alias +avoids both code generation and type inconsistencies. + +```ts +export const CipherType = { + Login: 1, + SecureNote: 2, + Card: 3, + Identity: 4, + SshKey: 5, +} as const; + +export type CipherType = (typeof CipherType)[keyof typeof CipherType]; + +// Can be imported together +import { CipherType } from "./cipher-type"; + +// Used as a type +function doSomething(type: CipherType) {} + +// And used as a value (just like a regular `enum`) +doSomething(CipherType.Card); +``` + +## Considered Options + +- **Allow enums, but advise against them** - This is the current state of affairs, as implemented by + the aforementioned PR. With this option, teams MUST address the readmes, but MAY address them by + disabling the lint. +- **Deprecate enum use** - Allow enums to exist for historic purposes, but prohibit the introduction + of new ones. Increase the lint to a "warning". This lets legacy code continue running + indefinitely, and would allow for cases where autogenerated code introduces an enum by necessity. +- **Eliminate enum use** - Prohibit the introduction of any new enum and replace all enums in the + codebase with typescript objects. Increase the lint to an "error". + +## Decision Outcome + +Chosen option: **Deprecate enum use** + +### Positive Consequences + +- Developers receive a warning in their IDE, which discourages new enums. +- The warning can direct them to our contributing docs, where they can learn typesafe alternatives. +- Over time, our code size decreases as enums are replaced. +- If all teams eliminate enums in practice, the warning can be increased to an error. + +### Negative Consequences + +- Unnecessary usage may persist indefinitely on teams carrying a high tech debt. +- The lint increases the number of FIXME comments in the code by about 10%. + +### Plan + +- Update contributing docs with patterns and best practices for enum replacement. +- Update the reporting level of the lint to "warning". diff --git a/docs/contributing/code-style/enums.md b/docs/contributing/code-style/enums.md index 5ed0044a5..36895eed2 100644 --- a/docs/contributing/code-style/enums.md +++ b/docs/contributing/code-style/enums.md @@ -1,25 +1,88 @@ -# TypeScript Const Enums +# Avoid TypeScript Enums ([ADR-0025](../../architecture/adr/0025-ts-deprecate-enums.md)) -We don't use Typescript enums because they are not fully type-safe and can cause surprises. Instead -of using enums, we use constant objects. +TypeScript enums are not fully type-safe and can [cause surprises](enum-surprises). Your code should +use [constant objects](constant-object-pattern) instead of introducing a new enum. ## Our Recommended Approach +- Use the same name for your type- and value-declaration. +- Use `type` to derive type information from the const object. +- Create utilities to convert and identify enums modelled as primitives. + +:::tip + +This pattern should simplify the usage of your new objects, improve type safety in files that have +adopted TS-strict, and make transitioning an enum to a const object much easier. + +::: + +### Example + +Given the following enum: + ```ts -export const PasskeyActions = { - Register: "register", - Authenticate: "authenticate", +export enum CipherType = { + Login: 1, + SecureNote: 2, + Card: 3, + Identity: 4, + SshKey: 5, +}; +``` + +You can redefine it as an object like so: + +```ts +const RawCipherType = { + Login: 1, + SecureNote: 2, + Card: 3, + Identity: 4, + SshKey: 5, } as const; -export type PasskeyActionValue = (typeof PasskeyActions)[keyof typeof PasskeyActions]; +type RawCipherType = typeof RawCipherType; -declare function usePasskeyAction(action: PasskeyActionValue): void; +export type CipherType = RawCipherType[keyof RawCipherType]; -usePasskey(PasskeyActions.Register); // ✅ Valid -usePasskey(0); // ❌ Invalid +export const CipherType: Readonly> = + Object.freeze(RawCipherType); ``` -## Resources Used +And use it like so: + +```ts +// Can be imported together +import { CipherType } from "./cipher-type"; + +// Used as a type +function doSomething(type: CipherType) {} + +// And used as a value (just like a regular `enum`) +doSomething(CipherType.Card); +``` + +The following utilities may assist introspection: + +```ts +import { CipherType } from "./cipher-type"; + +const namesByCipherType = new Map( + Array.fromEntries(Object.entries(CipherType), ([k, v]) => [v, k]), +); + +export function isCipherType(value: number): value is CipherType { + return namesByCipherType.has(value); +} + +export function asCipherType(value: number): CipherType | undefined { + return isCipherType(value) ? value : undefined; +} + +export function nameOfCipherType(name: CipherType): keyof CipherType | undefined { + return namesByCipherType.get(value); +} +``` -- https://dev.to/ivanzm123/dont-use-enums-in-typescript-they-are-very-dangerous-57bh -- https://www.typescriptlang.org/docs/handbook/enums.html#objects-vs-enums +[enum-surprises]: https://dev.to/ivanzm123/dont-use-enums-in-typescript-they-are-very-dangerous-57bh +[constant-object-pattern]: https://www.typescriptlang.org/docs/handbook/enums.html#objects-vs-enums From e7204121600e191fa0ed27216e1c05987ccafd4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=9C=A8=20Audrey=20=E2=9C=A8?= Date: Fri, 30 May 2025 12:46:13 -0400 Subject: [PATCH 02/13] fix broken hyperlink --- docs/architecture/adr/0025-ts-deprecate-enums.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/architecture/adr/0025-ts-deprecate-enums.md b/docs/architecture/adr/0025-ts-deprecate-enums.md index 7f82a2a8f..c2470f58d 100644 --- a/docs/architecture/adr/0025-ts-deprecate-enums.md +++ b/docs/architecture/adr/0025-ts-deprecate-enums.md @@ -86,3 +86,5 @@ Chosen option: **Deprecate enum use** - Update contributing docs with patterns and best practices for enum replacement. - Update the reporting level of the lint to "warning". + +[no-enum-lint]: https://github.com/bitwarden/clients/blob/main/libs/eslint/platform/no-enums.mjs From 603b6a1f6fa84d99131ea22b18a6ef1890fb9974 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=9C=A8=20Audrey=20=E2=9C=A8?= Date: Fri, 30 May 2025 12:50:02 -0400 Subject: [PATCH 03/13] use docusaurus link format --- docs/architecture/adr/0025-ts-deprecate-enums.md | 6 +++--- docs/contributing/code-style/enums.md | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/architecture/adr/0025-ts-deprecate-enums.md b/docs/architecture/adr/0025-ts-deprecate-enums.md index c2470f58d..42ca52258 100644 --- a/docs/architecture/adr/0025-ts-deprecate-enums.md +++ b/docs/architecture/adr/0025-ts-deprecate-enums.md @@ -25,9 +25,9 @@ worst cases, their limitations may be unknown, and thus unguarded. Reducing or e ### Detection -Typescript deprecation can be linted using a fairly short ESlint plugin. The code has -[already been contributed to main](no-enum-lint) as a suggestion. The same PR adds `FIXME` comments -for each team to address. +Typescript deprecation can be linted using a fairly short ESlint plugin. The code has [already been +contributed to main][no-enum-lint] as a suggestion. The same PR adds `FIXME` comments for each team +to address. ### Replacement code diff --git a/docs/contributing/code-style/enums.md b/docs/contributing/code-style/enums.md index 36895eed2..b8bc254bc 100644 --- a/docs/contributing/code-style/enums.md +++ b/docs/contributing/code-style/enums.md @@ -1,7 +1,7 @@ # Avoid TypeScript Enums ([ADR-0025](../../architecture/adr/0025-ts-deprecate-enums.md)) -TypeScript enums are not fully type-safe and can [cause surprises](enum-surprises). Your code should -use [constant objects](constant-object-pattern) instead of introducing a new enum. +TypeScript enums are not fully type-safe and can [cause surprises][enum-surprises]. Your code should +use [constant objects][constant-object-pattern] instead of introducing a new enum. ## Our Recommended Approach From f80e483016bf436f61ebc4377f15f12bfaf21333 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=9C=A8=20Audrey=20=E2=9C=A8?= Date: Fri, 30 May 2025 12:56:44 -0400 Subject: [PATCH 04/13] fix page title rendering error --- docs/contributing/code-style/enums.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/contributing/code-style/enums.md b/docs/contributing/code-style/enums.md index b8bc254bc..d66af6325 100644 --- a/docs/contributing/code-style/enums.md +++ b/docs/contributing/code-style/enums.md @@ -1,9 +1,9 @@ -# Avoid TypeScript Enums ([ADR-0025](../../architecture/adr/0025-ts-deprecate-enums.md)) +# Avoid TypeScript Enums TypeScript enums are not fully type-safe and can [cause surprises][enum-surprises]. Your code should use [constant objects][constant-object-pattern] instead of introducing a new enum. -## Our Recommended Approach +## Our Recommended Approach ([ADR-0025](../../architecture/adr/0025-ts-deprecate-enums.md)) - Use the same name for your type- and value-declaration. - Use `type` to derive type information from the const object. From 8d09eb4ef442a3bc530e5e4bdda1fac8f59c643f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=9C=A8=20Audrey=20=E2=9C=A8?= Date: Fri, 30 May 2025 13:08:00 -0400 Subject: [PATCH 05/13] adjust organization --- docs/architecture/adr/0025-ts-deprecate-enums.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/architecture/adr/0025-ts-deprecate-enums.md b/docs/architecture/adr/0025-ts-deprecate-enums.md index 42ca52258..1ab63ec4b 100644 --- a/docs/architecture/adr/0025-ts-deprecate-enums.md +++ b/docs/architecture/adr/0025-ts-deprecate-enums.md @@ -57,14 +57,13 @@ doSomething(CipherType.Card); ## Considered Options -- **Allow enums, but advise against them** - This is the current state of affairs, as implemented by - the aforementioned PR. With this option, teams MUST address the readmes, but MAY address them by - disabling the lint. +- **Allow enums, but advise against them** - This is the current state of affairs. With this option, + teams MUST address the readmes, but MAY address them by disabling the lint. - **Deprecate enum use** - Allow enums to exist for historic purposes, but prohibit the introduction - of new ones. Increase the lint to a "warning". This lets legacy code continue running - indefinitely, and would allow for cases where autogenerated code introduces an enum by necessity. + of new ones. Increase the lint to a "warning" and allow the lint to be disabled. - **Eliminate enum use** - Prohibit the introduction of any new enum and replace all enums in the - codebase with typescript objects. Increase the lint to an "error". + codebase with typescript objects. Increase the lint to an "error" and prohibit disabling of the + lint. ## Decision Outcome @@ -72,6 +71,7 @@ Chosen option: **Deprecate enum use** ### Positive Consequences +- Allows for cases where autogenerated code introduces an enum by necessity - Developers receive a warning in their IDE, which discourages new enums. - The warning can direct them to our contributing docs, where they can learn typesafe alternatives. - Over time, our code size decreases as enums are replaced. From 5459e979a2b4157069a9083ad53b3a1715dc8e49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=9C=A8=20Audrey=20=E2=9C=A8?= Date: Mon, 2 Jun 2025 07:53:11 -0400 Subject: [PATCH 06/13] revert enums.md; content moved to #607 --- docs/contributing/code-style/enums.md | 89 ++++----------------------- 1 file changed, 13 insertions(+), 76 deletions(-) diff --git a/docs/contributing/code-style/enums.md b/docs/contributing/code-style/enums.md index d66af6325..2283e94f9 100644 --- a/docs/contributing/code-style/enums.md +++ b/docs/contributing/code-style/enums.md @@ -1,88 +1,25 @@ -# Avoid TypeScript Enums +# TypeScript Const Enums -TypeScript enums are not fully type-safe and can [cause surprises][enum-surprises]. Your code should -use [constant objects][constant-object-pattern] instead of introducing a new enum. +We don't use Typescript enums because they are not fully type-safe and can cause surprises. Instead +of using enums, we use constant objects. ## Our Recommended Approach ([ADR-0025](../../architecture/adr/0025-ts-deprecate-enums.md)) -- Use the same name for your type- and value-declaration. -- Use `type` to derive type information from the const object. -- Create utilities to convert and identify enums modelled as primitives. - -:::tip - -This pattern should simplify the usage of your new objects, improve type safety in files that have -adopted TS-strict, and make transitioning an enum to a const object much easier. - -::: - -### Example - -Given the following enum: - ```ts -export enum CipherType = { - Login: 1, - SecureNote: 2, - Card: 3, - Identity: 4, - SshKey: 5, -}; -``` - -You can redefine it as an object like so: - -```ts -const RawCipherType = { - Login: 1, - SecureNote: 2, - Card: 3, - Identity: 4, - SshKey: 5, +export const PasskeyActions = { + Register: "register", + Authenticate: "authenticate", } as const; -type RawCipherType = typeof RawCipherType; +export type PasskeyActionValue = (typeof PasskeyActions)[keyof typeof PasskeyActions]; -export type CipherType = RawCipherType[keyof RawCipherType]; +declare function usePasskeyAction(action: PasskeyActionValue): void; -export const CipherType: Readonly> = - Object.freeze(RawCipherType); +usePasskey(PasskeyActions.Register); // ✅ Valid +usePasskey(0); // ❌ Invalid ``` -And use it like so: - -```ts -// Can be imported together -import { CipherType } from "./cipher-type"; - -// Used as a type -function doSomething(type: CipherType) {} - -// And used as a value (just like a regular `enum`) -doSomething(CipherType.Card); -``` - -The following utilities may assist introspection: - -```ts -import { CipherType } from "./cipher-type"; - -const namesByCipherType = new Map( - Array.fromEntries(Object.entries(CipherType), ([k, v]) => [v, k]), -); - -export function isCipherType(value: number): value is CipherType { - return namesByCipherType.has(value); -} - -export function asCipherType(value: number): CipherType | undefined { - return isCipherType(value) ? value : undefined; -} - -export function nameOfCipherType(name: CipherType): keyof CipherType | undefined { - return namesByCipherType.get(value); -} -``` +## Resources Used -[enum-surprises]: https://dev.to/ivanzm123/dont-use-enums-in-typescript-they-are-very-dangerous-57bh -[constant-object-pattern]: https://www.typescriptlang.org/docs/handbook/enums.html#objects-vs-enums +- https://dev.to/ivanzm123/dont-use-enums-in-typescript-they-are-very-dangerous-57bh +- https://www.typescriptlang.org/docs/handbook/enums.html#objects-vs-enums From 2b141dd74265953c55654c6cd51d7caab16ded61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=9C=A8=20Audrey=20=E2=9C=A8?= Date: Tue, 3 Jun 2025 12:10:38 -0400 Subject: [PATCH 07/13] grammar corrections --------- Co-authored-by: Matt Bishop --- custom-words.txt | 2 +- docs/architecture/adr/0025-ts-deprecate-enums.md | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/custom-words.txt b/custom-words.txt index 000141948..02a89ebf3 100644 --- a/custom-words.txt +++ b/custom-words.txt @@ -79,6 +79,7 @@ subprocessor toolset TOTP typealias +typesafe WCAG Xcodes.app xmldoc @@ -86,7 +87,6 @@ Yellowpages Yubico YubiKey YubiKeys -typesafe # Forbidden words !auto-fill diff --git a/docs/architecture/adr/0025-ts-deprecate-enums.md b/docs/architecture/adr/0025-ts-deprecate-enums.md index 1ab63ec4b..2484e1255 100644 --- a/docs/architecture/adr/0025-ts-deprecate-enums.md +++ b/docs/architecture/adr/0025-ts-deprecate-enums.md @@ -21,11 +21,11 @@ Among them: behavior of numeric enums. These inconsistencies cause increased complexity from guard statements in the best of cases. In the -worst cases, their limitations may be unknown, and thus unguarded. Reducing or eliminating +worst cases, their limitations may be unknown, and thus unguarded. ### Detection -Typescript deprecation can be linted using a fairly short ESlint plugin. The code has [already been +TypeScript deprecation can be linted using a fairly short ESLint plugin. The code has [already been contributed to main][no-enum-lint] as a suggestion. The same PR adds `FIXME` comments for each team to address. @@ -58,7 +58,7 @@ doSomething(CipherType.Card); ## Considered Options - **Allow enums, but advise against them** - This is the current state of affairs. With this option, - teams MUST address the readmes, but MAY address them by disabling the lint. + teams **must** address the readmes, but _may_ address them by disabling the lint. - **Deprecate enum use** - Allow enums to exist for historic purposes, but prohibit the introduction of new ones. Increase the lint to a "warning" and allow the lint to be disabled. - **Eliminate enum use** - Prohibit the introduction of any new enum and replace all enums in the @@ -71,8 +71,8 @@ Chosen option: **Deprecate enum use** ### Positive Consequences -- Allows for cases where autogenerated code introduces an enum by necessity -- Developers receive a warning in their IDE, which discourages new enums. +- Allows for cases where autogenerated code introduces an enum by necessity. +- Developers receive a warning in their IDE to discourage new enums. - The warning can direct them to our contributing docs, where they can learn typesafe alternatives. - Over time, our code size decreases as enums are replaced. - If all teams eliminate enums in practice, the warning can be increased to an error. From bcb820f8968ffcd2a8a0b24b209522008e0c466a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=9C=A8=20Audrey=20=E2=9C=A8?= Date: Tue, 3 Jun 2025 12:53:30 -0400 Subject: [PATCH 08/13] include negative consequences and workarounds --- .../adr/0025-ts-deprecate-enums.md | 71 ++++++++++++++++--- 1 file changed, 61 insertions(+), 10 deletions(-) diff --git a/docs/architecture/adr/0025-ts-deprecate-enums.md b/docs/architecture/adr/0025-ts-deprecate-enums.md index 2484e1255..30c93f30b 100644 --- a/docs/architecture/adr/0025-ts-deprecate-enums.md +++ b/docs/architecture/adr/0025-ts-deprecate-enums.md @@ -29,13 +29,14 @@ TypeScript deprecation can be linted using a fairly short ESLint plugin. The cod contributed to main][no-enum-lint] as a suggestion. The same PR adds `FIXME` comments for each team to address. -### Replacement code +### Replacement pattern In most cases, enums are unnecessary. A readonly (`as const`) object coupled with a type alias avoids both code generation and type inconsistencies. ```ts -export const CipherType = { +// declare the raw data and reduce repetition with an inner type +const _CipherType = { Login: 1, SecureNote: 2, Card: 3, @@ -43,22 +44,70 @@ export const CipherType = { SshKey: 5, } as const; -export type CipherType = (typeof CipherType)[keyof typeof CipherType]; +type _CipherType = typeof _CipherType; -// Can be imported together -import { CipherType } from "./cipher-type"; +// derive the enum-like type from the raw data +export type CipherType = _CipherType[keyof _CipherType]; -// Used as a type -function doSomething(type: CipherType) {} +// assert that the raw data is of the enum-like type +export const CipherType: Readonly<{ [K in keyof typeof _CipherType]: CipherType }> = + Object.freeze(_CipherType); +``` + +This code creates a type `CipherType` that allows arguments and variables to be typed similarly to +an enum. It also strongly types the `CiperType` constant so that direct accesses of its members +preserve type safety. No type assertions are needed to work directly with `CipherType`. -// And used as a value (just like a regular `enum`) -doSomething(CipherType.Card); +This pattern has two negative consequences. First, mapped types cannot determine that a mapped type +is fully specified. Code like the following causes a compiler error: + +```ts +type MappedType = { [K in CipherType]: boolean }; + +const instance: MappedType = { + [CipherType.Login]: true, + [CipherType.SecureNote]: true, + [CipherType.Card]: true, + [CipherType.Identity]: true, + [CipherType.SshKey]: true, +}; ``` +This happens because each enum members' literal type is overridden by `CipherType`. The compiler +cannot determine that every kind of `CipherType` is listed. There are a few workarounds: + +```ts +// option A: use a type assertion to construct the mapped type +const instance: MappedType = { + [CipherType.Login]: true, + // ... +} as MappedType; + +// option B: make the type partial and it fully typechecks, but you +// need to inspect its properties to use them. +type PartialMappedType = { [K in CipherType]?: boolean }; +const instance = { + [CipherType.Login]: true, + // ... +}; +if (instance[CipherType.Login]) { + // work with `instance` +} +``` + +The other problem is that mapped types cannot specify the enum-like's members as field names. Code +like the following causes a compiler error: + +```ts +type SomeType = { [CipherType.SecureNote]: boolean }; +``` + +[This issue is fixed as of TypeScript 5.8](no-member-fields-fixed). + ## Considered Options - **Allow enums, but advise against them** - This is the current state of affairs. With this option, - teams **must** address the readmes, but _may_ address them by disabling the lint. + teams **must** address the FIXMEs, but _may_ address them by disabling the lint. - **Deprecate enum use** - Allow enums to exist for historic purposes, but prohibit the introduction of new ones. Increase the lint to a "warning" and allow the lint to be disabled. - **Eliminate enum use** - Prohibit the introduction of any new enum and replace all enums in the @@ -88,3 +137,5 @@ Chosen option: **Deprecate enum use** - Update the reporting level of the lint to "warning". [no-enum-lint]: https://github.com/bitwarden/clients/blob/main/libs/eslint/platform/no-enums.mjs +[no-member-fields-fixed]: + https://devblogs.microsoft.com/typescript/announcing-typescript-5-8-beta/#preserved-computed-property-names-in-declaration-files From b126bdc58962cf2fef9d93574c46b14b08be2942 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=9C=A8=20Audrey=20=E2=9C=A8?= Date: Tue, 3 Jun 2025 12:58:30 -0400 Subject: [PATCH 09/13] minor lanuguage tweak --- docs/architecture/adr/0025-ts-deprecate-enums.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/architecture/adr/0025-ts-deprecate-enums.md b/docs/architecture/adr/0025-ts-deprecate-enums.md index 30c93f30b..aa91981a6 100644 --- a/docs/architecture/adr/0025-ts-deprecate-enums.md +++ b/docs/architecture/adr/0025-ts-deprecate-enums.md @@ -102,7 +102,8 @@ like the following causes a compiler error: type SomeType = { [CipherType.SecureNote]: boolean }; ``` -[This issue is fixed as of TypeScript 5.8](no-member-fields-fixed). +[This issue is fixed as of TypeScript 5.8](no-member-fields-fixed) and won't be explored further in +this document. ## Considered Options From 604d225f9606d07146dd0896a5f830747b30dcea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=9C=A8=20Audrey=20=E2=9C=A8?= Date: Tue, 3 Jun 2025 13:01:06 -0400 Subject: [PATCH 10/13] every time with the broken link format... --- docs/architecture/adr/0025-ts-deprecate-enums.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr/0025-ts-deprecate-enums.md b/docs/architecture/adr/0025-ts-deprecate-enums.md index aa91981a6..83e609e47 100644 --- a/docs/architecture/adr/0025-ts-deprecate-enums.md +++ b/docs/architecture/adr/0025-ts-deprecate-enums.md @@ -102,7 +102,7 @@ like the following causes a compiler error: type SomeType = { [CipherType.SecureNote]: boolean }; ``` -[This issue is fixed as of TypeScript 5.8](no-member-fields-fixed) and won't be explored further in +[This issue is fixed as of TypeScript 5.8][no-member-fields-fixed] and won't be explored further in this document. ## Considered Options From d504548a396cac9a2f2d60153e55ef5306fd8551 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=9C=A8=20Audrey=20=E2=9C=A8?= Date: Tue, 3 Jun 2025 18:41:28 -0400 Subject: [PATCH 11/13] lots of refinement --- custom-words.txt | 2 + .../adr/0025-ts-deprecate-enums.md | 156 ++++++++++++------ 2 files changed, 107 insertions(+), 51 deletions(-) diff --git a/custom-words.txt b/custom-words.txt index 02a89ebf3..01336186f 100644 --- a/custom-words.txt +++ b/custom-words.txt @@ -79,6 +79,8 @@ subprocessor toolset TOTP typealias +typecheck +typechecks typesafe WCAG Xcodes.app diff --git a/docs/architecture/adr/0025-ts-deprecate-enums.md b/docs/architecture/adr/0025-ts-deprecate-enums.md index 83e609e47..a17226dbd 100644 --- a/docs/architecture/adr/0025-ts-deprecate-enums.md +++ b/docs/architecture/adr/0025-ts-deprecate-enums.md @@ -29,13 +29,13 @@ TypeScript deprecation can be linted using a fairly short ESLint plugin. The cod contributed to main][no-enum-lint] as a suggestion. The same PR adds `FIXME` comments for each team to address. -### Replacement pattern +### The Enum-like Pattern In most cases, enums are unnecessary. A readonly (`as const`) object coupled with a type alias avoids both code generation and type inconsistencies. ```ts -// declare the raw data and reduce repetition with an inner type +// declare the raw data and reduce repetition with an internal type const _CipherType = { Login: 1, SecureNote: 2, @@ -50,67 +50,37 @@ type _CipherType = typeof _CipherType; export type CipherType = _CipherType[keyof _CipherType]; // assert that the raw data is of the enum-like type -export const CipherType: Readonly<{ [K in keyof typeof _CipherType]: CipherType }> = +export const CipherType: Readonly<{ [K in keyof _CipherType]: CipherType }> = Object.freeze(_CipherType); ``` -This code creates a type `CipherType` that allows arguments and variables to be typed similarly to -an enum. It also strongly types the `CiperType` constant so that direct accesses of its members -preserve type safety. No type assertions are needed to work directly with `CipherType`. - -This pattern has two negative consequences. First, mapped types cannot determine that a mapped type -is fully specified. Code like the following causes a compiler error: +This code creates a `type CipherType` that allows arguments and variables to be typed similarly to +an enum. It also strongly types the `const CiperType` so that direct accesses of its members +preserve type safety. This ensures that type inference properly limits the accepted values to those +allowed by `type CipherType`. Without the type assertion, the compiler infers `number` in these +cases: ```ts -type MappedType = { [K in CipherType]: boolean }; - -const instance: MappedType = { - [CipherType.Login]: true, - [CipherType.SecureNote]: true, - [CipherType.Card]: true, - [CipherType.Identity]: true, - [CipherType.SshKey]: true, -}; +const s = new Subject(CipherType.Login); // `s` is a `Subject` +const a = [CipherType.Login, CipherType.Card]; // `a` is an `Array` +const m = new Map([[CipherType.Login, ""]]); // `m` is a `Map` ``` -This happens because each enum members' literal type is overridden by `CipherType`. The compiler -cannot determine that every kind of `CipherType` is listed. There are a few workarounds: +:::warning -```ts -// option A: use a type assertion to construct the mapped type -const instance: MappedType = { - [CipherType.Login]: true, - // ... -} as MappedType; +- Types that use enums like [computed property names][computed-property-names] issue a compiler + error with this pattern. [This issue is fixed as of TypeScript 5.8][no-member-fields-fixed]. +- Certain objects are more difficult to create with this pattern. This is explored in + [Appendix A](#appendix-a-mapped-types-and-enum-likes). -// option B: make the type partial and it fully typechecks, but you -// need to inspect its properties to use them. -type PartialMappedType = { [K in CipherType]?: boolean }; -const instance = { - [CipherType.Login]: true, - // ... -}; -if (instance[CipherType.Login]) { - // work with `instance` -} -``` - -The other problem is that mapped types cannot specify the enum-like's members as field names. Code -like the following causes a compiler error: - -```ts -type SomeType = { [CipherType.SecureNote]: boolean }; -``` - -[This issue is fixed as of TypeScript 5.8][no-member-fields-fixed] and won't be explored further in -this document. +::: ## Considered Options - **Allow enums, but advise against them** - This is the current state of affairs. With this option, teams **must** address the FIXMEs, but _may_ address them by disabling the lint. -- **Deprecate enum use** - Allow enums to exist for historic purposes, but prohibit the introduction - of new ones. Increase the lint to a "warning" and allow the lint to be disabled. +- **Deprecate enum use** - Allow enums to exist for historic or technical purposes, but prohibit the + introduction of new ones. Increase the lint to a "warning" and allow the lint to be disabled. - **Eliminate enum use** - Prohibit the introduction of any new enum and replace all enums in the codebase with typescript objects. Increase the lint to an "error" and prohibit disabling of the lint. @@ -124,19 +94,103 @@ Chosen option: **Deprecate enum use** - Allows for cases where autogenerated code introduces an enum by necessity. - Developers receive a warning in their IDE to discourage new enums. - The warning can direct them to our contributing docs, where they can learn typesafe alternatives. -- Over time, our code size decreases as enums are replaced. +- Our compiled code size decreases when enums are replaced. - If all teams eliminate enums in practice, the warning can be increased to an error. ### Negative Consequences - Unnecessary usage may persist indefinitely on teams carrying a high tech debt. -- The lint increases the number of FIXME comments in the code by about 10%. +- The lint increased the number of FIXME comments in the code by about 10%. ### Plan - Update contributing docs with patterns and best practices for enum replacement. - Update the reporting level of the lint to "warning". +[computed-property-names]: + https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#computed_property_names +[literal-type]: https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#literal-types [no-enum-lint]: https://github.com/bitwarden/clients/blob/main/libs/eslint/platform/no-enums.mjs [no-member-fields-fixed]: https://devblogs.microsoft.com/typescript/announcing-typescript-5-8-beta/#preserved-computed-property-names-in-declaration-files + +## Appendix A: Mapped Types and Enum-likes + +Mapped types cannot determine that a mapped enum-like object is fully specified. Code like the +following causes a compiler error: + +```ts +const instance: Record = { + [CipherType.Login]: true, + [CipherType.SecureNote]: false, + [CipherType.Card]: true, + [CipherType.Identity]: true, + [CipherType.SshKey]: true, +}; +``` + +#### Why does this happen? + +The members of `const _CipherType` all have a literal type. `_CipherType.Login`, for example, has a +[literal type][literal-type] of `1`. `type CipherType` maps over these members, aggregating them +into the structural type `1 | 2 | 3 | 4 | 5`. + +`const CipherType` asserts its members have `type CipherType`, which overrides the literal types the +compiler inferred for the member in `const _CipherType`. The compiler sees the type of +`CipherType.Login` as `type CipherType` (which aliases `1 | 2 | 3 | 4 | 5`). + +Now consider a mapped type definition: + +```ts +// `MappedType` is structurally identical to Record +type MappedType = { [K in CipherType]: boolean }; +``` + +When the compiler examines `instance`, it only knows that the type of each of its members is +`CipherType`. That is, the type of `instance` to the compiler is +`{ [K in 1 | 2 | 3 | 4 | 5]?: boolean }`. This doesn't sufficiently overlap with `MappedType`, which +is looking for `{ [1]: boolean, [2]: boolean, [3]: boolean, [4]: boolean, [5]: boolean }`. The +failure occurs, because the inferred type can have fewer fields than `MappedType`. + +### Workarounds + +**Option A: Assert the type is correct.** You need to manually verify this. The compiler cannot +typecheck it. + +```ts +const instance: MappedType = { + [CipherType.Login]: true, + // ... +} as MappedType; +``` + +**Option B: Define the mapped type as a partial.** Then, inspect its properties before using them. + +```ts +type MappedType = { [K in CipherType]?: boolean }; +const instance: MappedType = { + [CipherType.Login]: true, + // ... +}; + +if (CipherType.Login in instance) { + // work with `instance[CipherType.Login]` +} +``` + +**Option C: Use a collection.** Consider this approach when downstream code reflects over the result +with `in` or using methods like `Object.keys`. + +```ts +const collection = new Map([[CipherType.Login, true]]); + +const instance = collection.get(CipherType.Login); +if (instance) { + // work with `instance` +} + +const available = [CipherType.Login, CipherType.Card]; +if (available.includes(CipherType.Login)) { + // ... +} +``` From d3e7d474e8da89733bb0343263555fd3dd2e70ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=9C=A8=20Audrey=20=E2=9C=A8?= Date: Tue, 3 Jun 2025 18:43:32 -0400 Subject: [PATCH 12/13] fix literal type link --- docs/architecture/adr/0025-ts-deprecate-enums.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/architecture/adr/0025-ts-deprecate-enums.md b/docs/architecture/adr/0025-ts-deprecate-enums.md index a17226dbd..3415c628f 100644 --- a/docs/architecture/adr/0025-ts-deprecate-enums.md +++ b/docs/architecture/adr/0025-ts-deprecate-enums.md @@ -116,7 +116,7 @@ Chosen option: **Deprecate enum use** ## Appendix A: Mapped Types and Enum-likes -Mapped types cannot determine that a mapped enum-like object is fully specified. Code like the +Mapped types cannot determine that a mapped enum-like object is fully assigned. Code like the following causes a compiler error: ```ts @@ -131,9 +131,9 @@ const instance: Record = { #### Why does this happen? -The members of `const _CipherType` all have a literal type. `_CipherType.Login`, for example, has a -[literal type][literal-type] of `1`. `type CipherType` maps over these members, aggregating them -into the structural type `1 | 2 | 3 | 4 | 5`. +The members of `const _CipherType` all have a [literal type][literal-type]. `_CipherType.Login`, for +example, has a literal type of `1`. `type CipherType` maps over these members, aggregating them into +the structural type `1 | 2 | 3 | 4 | 5`. `const CipherType` asserts its members have `type CipherType`, which overrides the literal types the compiler inferred for the member in `const _CipherType`. The compiler sees the type of From 58e517790ff5e02f2100dea00bcb4d9e067f0e6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=9C=A8=20Audrey=20=E2=9C=A8?= Date: Wed, 4 Jun 2025 11:05:14 -0400 Subject: [PATCH 13/13] revise current state description --- .../adr/0025-ts-deprecate-enums.md | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/docs/architecture/adr/0025-ts-deprecate-enums.md b/docs/architecture/adr/0025-ts-deprecate-enums.md index 3415c628f..25bf34d2f 100644 --- a/docs/architecture/adr/0025-ts-deprecate-enums.md +++ b/docs/architecture/adr/0025-ts-deprecate-enums.md @@ -26,8 +26,8 @@ worst cases, their limitations may be unknown, and thus unguarded. ### Detection TypeScript deprecation can be linted using a fairly short ESLint plugin. The code has [already been -contributed to main][no-enum-lint] as a suggestion. The same PR adds `FIXME` comments for each team -to address. +contributed to main][no-enum-lint] and is configured as [an error-level +lint][no-enum-configuration]. The same PR adds `FIXME` comments for each team to address. ### The Enum-like Pattern @@ -77,12 +77,12 @@ const m = new Map([[CipherType.Login, ""]]); // `m` is a `Map