Introduce ADR-0025: deprecate typescript enums

[audreyality]

🎟️ Tracking

N/A

📔 Objective

Document decision to deprecate typescript enums across the bitwarden/clients codebase.

(Guidelines - rendered, ADR - rendered)

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
  issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[cloudflare-workers-and-pages]

Deploying contributing-docs with    Cloudflare Pages

Latest commit:	58e5177
Status:	✅  Deploy successful!
Preview URL:	https://e1b5014a.contributing-docs.pages.dev
Branch Preview URL:	https://deprecate-ts-enums-adr.contributing-docs.pages.dev

View logs

[audreyality]

📝 Our discussions to date have indicated that our Rust code can still generate enums, so this document assumes that we can deprecate but not eliminate enums.

💭 I'm not sure if we should include direction for the SDK in this. Per @dani-garcia rust-bindgen produces typescript enums and we can override this with tsify. The "deprecate" option includes an exception made for this use-case. If we want to deprecate these uses too, then I think we'd want to go with the "eliminate" option and expand the scope of the ADR to include the rust/SDK changes.

[dani-garcia]

So I went to look a bit more into what wasm-bindgen is doing internally when not using tsify, and this is what it generates:

In *.d.ts file:

export enum LogLevel {
  Trace = 0,
   ... all other variants
}

In the *.js file:

/**
 * @enum {0 | 1 | 2 | 3 | 4}
 */
export const LogLevel = Object.freeze({
  Trace: 0,
  0: "Trace",
  ... all other variants
});

Not sure why they have that mapping from the number back to the string value, but other than that this seems fairly similar to what we want (but without the type alias).

I still think we should be encouraging using full discriminated unions with tsify, though.

[audreyality]

Not sure why they have that mapping from the number back to the string value, but other than that this seems fairly similar to what we want (but without the type alias).

It's maintaining compatibility with TypeScript. TS enums support bidirectional mapping. You can use the symbol name to get the value or the value to get the symbol name.

The pattern that @Patrick-Pimentel-Bitwarden, @coroiu, and I have been working on moves the name mapping into a utility function so that we can provide proper error handling. It's been extracted from this PR and moved to #607, since the ADR has to go through the architecture council.

[withinfocus]

I feel this works as a provisional first pass and could be merged as proposed, but up to you if you want to take in more feedback. Said feedback can also come as a result of the council session.

[coroiu]

Overall this looks really good, just one minor issue and then one thought about our recommended approach

[coroiu]

issue?: I think the route we took is between the definitions of "Deprecate" and "Eliminate". We made the lint rule throw errors (not warnings) but we also allowed and did disable those errors for existing cases

[audreyality]

According to the code, it's presently a suggestion-level lint.

[coroiu]

Oh lol that's confusing, it's an error if you look in the config: https://github.com/bitwarden/clients/blob/032fedf308ec251f17632d7d08c4daf6f41a4b1d/eslint.config.mjs#L77

[audreyality]

JavaScript is really embracing it's namesake here...

Addressed in 58e5177.

[coroiu]

Or are you maybe suggesting a change to our current approach, i.e. to downgrade error -> warning

[audreyality]

Addressed in 58e5177.

[coroiu]

thought: I feel like none of these workarounds actually fix the issue which causes less than possible static type-safety. Are we doing this because we value consistency more than the possibility of statically checked mapped types? Should we consider an "escape hatch" that allows access to the literally typed member in our recommended approach?

const instance: Record<CipherType, boolean> = {
  [CipherType[Literal].Login]: true,
  [CipherType[Literal].SecureNote]: false,
  [CipherType[Literal].Card]: true,
  [CipherType[Literal].Identity]: true,
  [CipherType[Literal].SshKey]: true,
};

I guess this is a perfect topic for the arch meeting :)

[audreyality]

👍🏻 Before I respond with my opinions, I totally agree this is why we need the arch meeting.

I feel like none of these workarounds actually fix the issue which causes less than possible static type-safety.

⛏️ That's what makes them a workaround. If they fixed the issue, they'd just be part of the documented solution. 😉

Are we doing this because we value consistency more than the possibility of statically checked mapped types?

🤔 I'd characterize this differently:

1. This proposal assumes that correctly inferring the enum type in low-level scenarios is preferable to the compiler inferring the type to be number.
2. It further assumes that losing type safety when constructing mapped types using literals is an acceptable tradeoff.

And as to why I think this is justified?

🎨 The most important aspect to me is that these types produce an awful developer experience. The instantiated type loses the symbols completely, so your debugger will show the object like this:

{ 1: true, 2: false, 3: true, 4: true, 5: true }

In practice it's difficult to get the enum definition in scope, so you just gotta know what symbol goes with each of those numbers.

♻️ I suspect a more compelling case is that the enum-like pattern without the typecast still has type assertions that can't be guaranteed by the compiler, they're just easier to gloss over. In the following code, wrong and invalid infer CipherType even though 8 is invalid due to an incorrect type assertion.

const wrong = [1, 2, 8] as CipherType[];
const invalid = [CipherType.Login, 2, 8] as CipherType[];

Without the type assertions, the statements typecheck as the correct type, number. Critically, an array composed entirely from CipherType typechecks as CipherType!

const numberArray = [1, 2, 8];
const alsoANumberArray = [CipherType.Login, 2, 8];

const cipherTypeArray = [CipherType.Login, CipherType.Card];

This matters for when you go to use the array later. In the following code, typecheckFails causes a type error because number[] cannot be assigned to cipherType. Attempting to assign wrong or invalid passes the typechecker.

// compiler error
const typecheckFails: CipherType[] = numberArray;

// totally fine
const wrongAssignment: CipherType[] = wrong;
const invalidAssignment: CipherType[] = invalid;

// the correct code asserts the proper type using a type predicate
function isCipherType(value: number): value is CipherType {
  return Object.values(_CipherType).includes(value as any);
}

const correct = numberArray.filter(isCipherType);

📝 The kicker is that in the code I've seen from Vault and Tools, the invalid as CipherType code shows up more frequently than types that map over enums.

Important

I didn't do a full review of our code, so I welcome data that proves me wrong here.

Should we consider an "escape hatch" that allows access to the literally typed member in our recommended approach?

I think adding that in causes us to continue repeating bad designs. If we want this capability, we need to admit that enums solve it better than an enum-like ever will.

[withinfocus]

⛏️ This would be present tense; don't understand the change.

[audreyality]

Andreas pointed out that our current approach configures the lint as an error. When I wrote this, I'd gathered it was only a suggestion. The lint is already active in a more restrictive capacity than this ADR proposes--so strict that the code would not build without the FIXMEs--hence this consequence in the past tense.

[withinfocus]

I'll leave final acceptance to the engineers working through the code suggestions here.

[withinfocus]

(auto-merge kills me in these situations)

[audreyality]

My bad. I figured the remaining suggestions were open questions for the architecture council. 🫠

[withinfocus]

I just wanted an engineer to give the final blessing. We'll get to any edits at the council session.