Skip to content

Commit d504548

Browse files
committed
lots of refinement
1 parent 604d225 commit d504548

File tree

2 files changed

+107
-51
lines changed

2 files changed

+107
-51
lines changed

custom-words.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,8 @@ subprocessor
7979
toolset
8080
TOTP
8181
typealias
82+
typecheck
83+
typechecks
8284
typesafe
8385
WCAG
8486
Xcodes.app

docs/architecture/adr/0025-ts-deprecate-enums.md

Lines changed: 105 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,13 @@ TypeScript deprecation can be linted using a fairly short ESLint plugin. The cod
2929
contributed to main][no-enum-lint] as a suggestion. The same PR adds `FIXME` comments for each team
3030
to address.
3131

32-
### Replacement pattern
32+
### The Enum-like Pattern
3333

3434
In most cases, enums are unnecessary. A readonly (`as const`) object coupled with a type alias
3535
avoids both code generation and type inconsistencies.
3636

3737
```ts
38-
// declare the raw data and reduce repetition with an inner type
38+
// declare the raw data and reduce repetition with an internal type
3939
const _CipherType = {
4040
Login: 1,
4141
SecureNote: 2,
@@ -50,67 +50,37 @@ type _CipherType = typeof _CipherType;
5050
export type CipherType = _CipherType[keyof _CipherType];
5151

5252
// assert that the raw data is of the enum-like type
53-
export const CipherType: Readonly<{ [K in keyof typeof _CipherType]: CipherType }> =
53+
export const CipherType: Readonly<{ [K in keyof _CipherType]: CipherType }> =
5454
Object.freeze(_CipherType);
5555
```
5656

57-
This code creates a type `CipherType` that allows arguments and variables to be typed similarly to
58-
an enum. It also strongly types the `CiperType` constant so that direct accesses of its members
59-
preserve type safety. No type assertions are needed to work directly with `CipherType`.
60-
61-
This pattern has two negative consequences. First, mapped types cannot determine that a mapped type
62-
is fully specified. Code like the following causes a compiler error:
57+
This code creates a `type CipherType` that allows arguments and variables to be typed similarly to
58+
an enum. It also strongly types the `const CiperType` so that direct accesses of its members
59+
preserve type safety. This ensures that type inference properly limits the accepted values to those
60+
allowed by `type CipherType`. Without the type assertion, the compiler infers `number` in these
61+
cases:
6362

6463
```ts
65-
type MappedType = { [K in CipherType]: boolean };
66-
67-
const instance: MappedType = {
68-
[CipherType.Login]: true,
69-
[CipherType.SecureNote]: true,
70-
[CipherType.Card]: true,
71-
[CipherType.Identity]: true,
72-
[CipherType.SshKey]: true,
73-
};
64+
const s = new Subject(CipherType.Login); // `s` is a `Subject<CipherType>`
65+
const a = [CipherType.Login, CipherType.Card]; // `a` is an `Array<CipherType>`
66+
const m = new Map([[CipherType.Login, ""]]); // `m` is a `Map<CipherType, string>`
7467
```
7568

76-
This happens because each enum members' literal type is overridden by `CipherType`. The compiler
77-
cannot determine that every kind of `CipherType` is listed. There are a few workarounds:
69+
:::warning
7870

79-
```ts
80-
// option A: use a type assertion to construct the mapped type
81-
const instance: MappedType = {
82-
[CipherType.Login]: true,
83-
// ...
84-
} as MappedType;
71+
- Types that use enums like [computed property names][computed-property-names] issue a compiler
72+
error with this pattern. [This issue is fixed as of TypeScript 5.8][no-member-fields-fixed].
73+
- Certain objects are more difficult to create with this pattern. This is explored in
74+
[Appendix A](#appendix-a-mapped-types-and-enum-likes).
8575

86-
// option B: make the type partial and it fully typechecks, but you
87-
// need to inspect its properties to use them.
88-
type PartialMappedType = { [K in CipherType]?: boolean };
89-
const instance = {
90-
[CipherType.Login]: true,
91-
// ...
92-
};
93-
if (instance[CipherType.Login]) {
94-
// work with `instance`
95-
}
96-
```
97-
98-
The other problem is that mapped types cannot specify the enum-like's members as field names. Code
99-
like the following causes a compiler error:
100-
101-
```ts
102-
type SomeType = { [CipherType.SecureNote]: boolean };
103-
```
104-
105-
[This issue is fixed as of TypeScript 5.8][no-member-fields-fixed] and won't be explored further in
106-
this document.
76+
:::
10777

10878
## Considered Options
10979

11080
- **Allow enums, but advise against them** - This is the current state of affairs. With this option,
11181
teams **must** address the FIXMEs, but _may_ address them by disabling the lint.
112-
- **Deprecate enum use** - Allow enums to exist for historic purposes, but prohibit the introduction
113-
of new ones. Increase the lint to a "warning" and allow the lint to be disabled.
82+
- **Deprecate enum use** - Allow enums to exist for historic or technical purposes, but prohibit the
83+
introduction of new ones. Increase the lint to a "warning" and allow the lint to be disabled.
11484
- **Eliminate enum use** - Prohibit the introduction of any new enum and replace all enums in the
11585
codebase with typescript objects. Increase the lint to an "error" and prohibit disabling of the
11686
lint.
@@ -124,19 +94,103 @@ Chosen option: **Deprecate enum use**
12494
- Allows for cases where autogenerated code introduces an enum by necessity.
12595
- Developers receive a warning in their IDE to discourage new enums.
12696
- The warning can direct them to our contributing docs, where they can learn typesafe alternatives.
127-
- Over time, our code size decreases as enums are replaced.
97+
- Our compiled code size decreases when enums are replaced.
12898
- If all teams eliminate enums in practice, the warning can be increased to an error.
12999

130100
### Negative Consequences
131101

132102
- Unnecessary usage may persist indefinitely on teams carrying a high tech debt.
133-
- The lint increases the number of FIXME comments in the code by about 10%.
103+
- The lint increased the number of FIXME comments in the code by about 10%.
134104

135105
### Plan
136106

137107
- Update contributing docs with patterns and best practices for enum replacement.
138108
- Update the reporting level of the lint to "warning".
139109

110+
[computed-property-names]:
111+
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer#computed_property_names
112+
[literal-type]: https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#literal-types
140113
[no-enum-lint]: https://github.com/bitwarden/clients/blob/main/libs/eslint/platform/no-enums.mjs
141114
[no-member-fields-fixed]:
142115
https://devblogs.microsoft.com/typescript/announcing-typescript-5-8-beta/#preserved-computed-property-names-in-declaration-files
116+
117+
## Appendix A: Mapped Types and Enum-likes
118+
119+
Mapped types cannot determine that a mapped enum-like object is fully specified. Code like the
120+
following causes a compiler error:
121+
122+
```ts
123+
const instance: Record<CipherType, boolean> = {
124+
[CipherType.Login]: true,
125+
[CipherType.SecureNote]: false,
126+
[CipherType.Card]: true,
127+
[CipherType.Identity]: true,
128+
[CipherType.SshKey]: true,
129+
};
130+
```
131+
132+
#### Why does this happen?
133+
134+
The members of `const _CipherType` all have a literal type. `_CipherType.Login`, for example, has a
135+
[literal type][literal-type] of `1`. `type CipherType` maps over these members, aggregating them
136+
into the structural type `1 | 2 | 3 | 4 | 5`.
137+
138+
`const CipherType` asserts its members have `type CipherType`, which overrides the literal types the
139+
compiler inferred for the member in `const _CipherType`. The compiler sees the type of
140+
`CipherType.Login` as `type CipherType` (which aliases `1 | 2 | 3 | 4 | 5`).
141+
142+
Now consider a mapped type definition:
143+
144+
```ts
145+
// `MappedType` is structurally identical to Record<CipherType, boolean>
146+
type MappedType = { [K in CipherType]: boolean };
147+
```
148+
149+
When the compiler examines `instance`, it only knows that the type of each of its members is
150+
`CipherType`. That is, the type of `instance` to the compiler is
151+
`{ [K in 1 | 2 | 3 | 4 | 5]?: boolean }`. This doesn't sufficiently overlap with `MappedType`, which
152+
is looking for `{ [1]: boolean, [2]: boolean, [3]: boolean, [4]: boolean, [5]: boolean }`. The
153+
failure occurs, because the inferred type can have fewer fields than `MappedType`.
154+
155+
### Workarounds
156+
157+
**Option A: Assert the type is correct.** You need to manually verify this. The compiler cannot
158+
typecheck it.
159+
160+
```ts
161+
const instance: MappedType = {
162+
[CipherType.Login]: true,
163+
// ...
164+
} as MappedType;
165+
```
166+
167+
**Option B: Define the mapped type as a partial.** Then, inspect its properties before using them.
168+
169+
```ts
170+
type MappedType = { [K in CipherType]?: boolean };
171+
const instance: MappedType = {
172+
[CipherType.Login]: true,
173+
// ...
174+
};
175+
176+
if (CipherType.Login in instance) {
177+
// work with `instance[CipherType.Login]`
178+
}
179+
```
180+
181+
**Option C: Use a collection.** Consider this approach when downstream code reflects over the result
182+
with `in` or using methods like `Object.keys`.
183+
184+
```ts
185+
const collection = new Map([[CipherType.Login, true]]);
186+
187+
const instance = collection.get(CipherType.Login);
188+
if (instance) {
189+
// work with `instance`
190+
}
191+
192+
const available = [CipherType.Login, CipherType.Card];
193+
if (available.includes(CipherType.Login)) {
194+
// ...
195+
}
196+
```

0 commit comments

Comments
 (0)