Initial pass of documentation on enums #576
Conversation
Deploying contributing-docs with
|
| Latest commit: |
69a927a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0512c9c6.contributing-docs.pages.dev |
| Branch Preview URL: | https://notes-on-enums.contributing-docs.pages.dev |
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
Hinton
left a comment
Hinton
left a comment
There was a problem hiding this comment.
Some initial thoughts. I'm split on if this belongs in architecture or code style under contribution.
Please review the rendered output https://notes-on-enums.contributing-docs.pages.dev/architecture/clients/enums.
withinfocus
changed the title
|
See comments on bitwarden/clients#14347 too. I am also of the opinion that this could just end up in code style. |
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
This reads more like an ADR to me, going into the background for the decision. For Code Style, I think it can be more succinct, e.g. "We don't use Typescript enums because they are not fully typesafe and can cause surprises. Instead of using enums, we use constant objects... [continue with Our Recommended Approach]". I think the links down the bottom are good too. But the intro, reasons, and problems sections are unnecessary imo. Code Style should be short and instructional.
|
I abandoned the context and left it with just the recommended approach. |
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
My comments have been addressed but I'll let the other reviewers give the final approval once they're happy.
| SshKey: 5, | ||
| } as const; | ||
|
|
||
| export type CipherTypeValue = (typeof CipherTypes)[keyof typeof CipherTypes]; |
There was a problem hiding this comment.
💭 (non-blocking) It would be useful to have a utility type for this.
There was a problem hiding this comment.
Would you mind providing your justification?
There was a problem hiding this comment.
Because every time you declare a const object to act as an enum, you're going to need to repeat this code:
export type CipherTypeValue = (typeof CipherTypes)[keyof typeof CipherTypes];And aside from how much copy/paste that is, I personally can never remember the keyof typeof incantation the first time. It's low level type mapping to have to do every time you declare an enum this way, and the TS syntax is pretty opaque.
Whereas you could do this:
// write this once
export type GetObjectKeys<T> = T[keyof T];
// then when I add an enum, all I need to do is:
// import { GetObjectKeys } ...
export type CipherTypeValue = GetObjectKeys<typeof CipherTypes>;There was a problem hiding this comment.
okay! I will make this addition in a fast follow to bitwarden/clients#14650 so not to have to re-request reviews and update the docs. Thanks for the callout.
|
|
||
| - 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 | ||
| - https://devblogs.microsoft.com/typescript/announcing-typescript-5-8-beta/#the---erasablesyntaxonly-optio |
There was a problem hiding this comment.
typo
| - https://devblogs.microsoft.com/typescript/announcing-typescript-5-8-beta/#the---erasablesyntaxonly-optio | |
| - https://devblogs.microsoft.com/typescript/announcing-typescript-5-8-beta/#the---erasablesyntaxonly-option |
There was a problem hiding this comment.
We could potentially drop this link; it's more about options for disabling typescript-specific syntax (of which enums is an example) than reasoning for the adoption of the code style policy 🤷
| export const CipherTypes = { | ||
| Login: 1, | ||
| SecureNote: 2, | ||
| Card: 3, | ||
| Identity: 4, | ||
| SshKey: 5, | ||
| } as const; | ||
|
|
||
| export type CipherTypeValue = (typeof CipherTypes)[keyof typeof CipherTypes]; | ||
|
|
||
| declare function useCipher(type: CipherTypeValue): void; | ||
|
|
||
| useCipher(CipherTypes.Login); // ✅ Valid | ||
| useCipher(42); // ❌ Invalid |
There was a problem hiding this comment.
non-blocking nit:
A different example might be helpful here to avoid conflating the concept of the data model "type" (kind) attribute and the TS type concept
| export const CipherTypes = { | |
| Login: 1, | |
| SecureNote: 2, | |
| Card: 3, | |
| Identity: 4, | |
| SshKey: 5, | |
| } as const; | |
| export type CipherTypeValue = (typeof CipherTypes)[keyof typeof CipherTypes]; | |
| declare function useCipher(type: CipherTypeValue): void; | |
| useCipher(CipherTypes.Login); // ✅ Valid | |
| useCipher(42); // ❌ Invalid | |
| export const PasskeyActions = { | |
| Register: "register", | |
| Authenticate: "authenticate", | |
| } as const; | |
| export type PasskeyActionValue = (typeof PasskeyActions)[keyof typeof PasskeyActions]; | |
| declare function usePasskeyAction(action: PasskeyActionValue): void; | |
| usePasskey(PasskeyActions.Register); // ✅ Valid | |
| usePasskey(42); // ❌ Invalid |
🎟️ Tracking
📔 Objective
Adding documentation to point to when making the change for enums.
⏰ Reminders before review
team
🦮 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 confirmedissue 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