add document for typescript guidelines #80
Conversation
There was a problem hiding this comment.
Hi, great work putting this together!
I noticed that you included my work in this (#69 (comment)). These entries are still WIP, and not intended to be published yet. I'd appreciate if you could remove them. I'll submit them when they're ready.
Also, a heads-up that my PR for #47 is almost ready for review, so any content here relating to any, type assertions, type annotations may be edited to remove duplication and make the doc read coherently.
DDDDDanica
force-pushed
the
feature/69-typescript
branch
from
ae1db34
to
c87162c
Compare
|
Hey @MajorLift thanks for your review, when i was taking the ticket, i just collected the points in this ticket and didn't notice your work, will remove now |
DDDDDanica
force-pushed
the
feature/69-typescript
branch
from
c87162c
to
c8645f5
Compare
There was a problem hiding this comment.
@DDDDDanica No worries, thanks for responding quickly! I had a few more suggestions and corrections:
| #### Avoid type annotations | ||
|
|
||
| Type annotations act like contracts with the compiler, ensuring a variable sticks to a specific data type its entire life. | ||
|
|
||
| In rare cases, using type annotations with `reduce` can be beneficial. It helps tp clarify complex accumulator types, enhance readability and ensure the compiler understands its intended use. | ||
|
|
||
| If you need to create a data structure with specific values, but ensure that it's declared with a more abstract type. Create a function that takes an argument of the type you want, then use it to build the object. | ||
|
|
||
| ```typescript | ||
| interface Person { | ||
| name: string; | ||
| age: number; | ||
| occupation?: string; // Optional property | ||
| } | ||
|
|
||
| // Function with type annotations: | ||
| function createPerson<T extends { name: string; age: number }>( | ||
| data: T, | ||
| ): Person { | ||
| // Infer types from the data argument | ||
| const { name, age, occupation = 'Unknown' } = data; | ||
|
|
||
| // Ensure properties adhere to the Person interface | ||
| return { name, age, occupation }; | ||
| } | ||
|
|
||
| // Usage: | ||
| const person1 = createPerson({ name: 'Alice', age: 30 }); // { name: "Alice", age: 30, occupation: "Unknown" } | ||
| const person2 = createPerson({ name: 'Bob', age: 25, occupation: 'Developer' }); // { name: "Bob", age: 25, occupation: "Developer" } | ||
| ``` | ||
|
|
||
| TypeScript is planning to introduce the `satisfies` keyword, which will provide a more flexible way to declare that a data structure conforms to a specific type without sacrificing readability or maintainability. However, this feature is not yet available. |
There was a problem hiding this comment.
Type annotations prevent inference-based narrowing of the user-supplied type. This is why they should be avoided, or at least why type inference should generally be preferred (unless supplying the annotation narrows the type further than the inferred result).
In contrast, the satisfies operator enables the user to assign a wider/abstract type to a value as a constraint, while also allowing the value's type to be further narrowed.
It lets us avoid wrapping specific types in a function to assign a wider type.
Need to create a data structure with specific values, but ensure that it's declared with a more abstract type? Create a function that takes an argument of the type you want, then use it to build the object. (#69 (comment))
const limitExceeded = () =>
({
jsonrpc: '2.0',
id: IdGenerator(),
error: {
code: -32005,
message: 'Limit exceeded',
},
}),I'll update this section with my PR for #47, since I already cover these points there.
DDDDDanica
force-pushed
the
feature/69-typescript
branch
from
e5ab3b7
to
5febd72
Compare
docs/typescript.md
Outdated
| ## **Type** | ||
|
|
||
| By defining expected object structures with interfaces, TypeScript promotes developer intent and unlocks powerful IDE features like accurate completion and context-aware navigation. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Removing the second entry is also acceptable since @typescript-eslint/consistent-type-definitions does a good job of enforcing the practice without everyone having to know the reasoning behind its adoption.
(The following is just me writing out my thoughts, not aimed at you in particular)
In general, the guidelines doesn't need to cover conventions that we already enforce using linter rules or static code analysis in too much detail. This doc isn't intended as a comprehensive collection of every practice or preference we have agreed upon. The priority should be on covering principles or processes that aren't reducible to simple rulesets and rely on a deeper understanding from the developers to be effective.
There was a problem hiding this comment.
Thanks for all the recommendations, i've added your suggestions, and changed the section title as Implements
There was a problem hiding this comment.
Looks like we're almost there! Just one important fix and some linting issues left.
On the subject of linting, I recommend setting up the markdownlint extension in your IDE. Its ruleset is mostly consistent with our prettier config and a lot more comprehensive.
Having a shared set of conventions on how to structure and format these docs helps future collaborators add their contributions without feeling like they're stepping on any toes.
On why consistency in the heading hierarchy is a good thing, check out the outline view in the preview for this doc: https://github.com/MetaMask/contributor-docs/blob/feature/69-typescript/docs/typescript.md. You can enable it by clicking on the unordered-list icon on the top right (Button.types__StyledButton-sc-ws60qy-0).
I think we should be ready to merge after this next round of fixes. Thanks for bearing with me through all these comments.
DDDDDanica
force-pushed
the
feature/69-typescript
branch
from
bb90285
to
922eccb
Compare
| #### Avoid type assertions, or explain their purpose whenever necessary. | ||
|
|
||
| Type assertions inform TypeScript about a variable's actual type, using the `as` syntax. | ||
| While type assertions offer flexibility, they can bypass TypeScript's safety net. This means the compiler won't catch potential type mismatches, leaving you responsible for ensuring correctness. | ||
|
|
||
| While it's generally recommended to avoid type assertions in TypeScript, here are two common scenarios where they might be necessary: | ||
|
|
||
| 1. Using reduce: | ||
|
|
||
| Imagine you have an array of User objects and want to find the youngest user using reduce. You start with an empty accumulator `({})`, but the final result should be a User object. | ||
|
|
||
| ```typescript | ||
| interface User { | ||
| name: string; | ||
| age: number; | ||
| } | ||
|
|
||
| const users: User[] = [ | ||
| // ... user data | ||
| ]; | ||
|
|
||
| const youngestUser = users.reduce((acc, user) => { | ||
| if (!acc.age || user.age < acc.age) { | ||
| acc = user; // Here, `acc` might be undefined initially. | ||
| } | ||
| return acc; | ||
| }, {} as User); // Assert `{}` as `User` to satisfy the return type. | ||
|
|
||
| console.log(youngestUser); // { name: "...", age: maxAge } | ||
| ``` | ||
|
|
||
| 2. Iterating over Enum Keys: | ||
| When iterating over enum keys using `Object.keys`, the resulting array is of type `string[]`. However, if you want to access enum member values directly, you might need a type assertion. | ||
|
|
||
| ```typescript | ||
| enum Direction { | ||
| Up = 'up', | ||
| Down = 'down', | ||
| Left = 'left', | ||
| Right = 'right', | ||
| } | ||
| const directions: Direction[] = [Direction.Up, Direction.Down]; | ||
| for (const key of Object.keys(directions)) { | ||
| const direction = directions[key as keyof typeof directions]; | ||
| console.log(direction); // Output: Direction.Up, Direction.Down } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Heads-up that I'll be integrating this section into https://github.com/MetaMask/contributor-docs/pull/74/files#diff-ae03a9f2eb638a25c1d8d7ba853c78220f1ed44a19ad97a8d780cbc0c00a4260R161-R239, since this topic falls under the scope of #47.
| #### Avoid type annotations | ||
|
|
||
| Type annotations act like contracts with the compiler, ensuring a variable sticks to a specific data type its entire life. | ||
|
|
||
| In rare cases, using type annotations with `reduce` can be beneficial. It helps tp clarify complex accumulator types, enhance readability and ensure the compiler understands its intended use. | ||
|
|
||
| If you need to create a data structure with specific values, but ensure that it's declared with a more abstract type. Create a function that takes an argument of the type you want, then use it to build the object. | ||
|
|
||
| ```typescript | ||
| interface Person { | ||
| name: string; | ||
| age: number; | ||
| occupation?: string; // Optional property | ||
| } | ||
|
|
||
| // Function with type annotations: | ||
| function createPerson<T extends { name: string; age: number }>( | ||
| data: T, | ||
| ): Person { | ||
| // Infer types from the data argument | ||
| const { name, age, occupation = 'Unknown' } = data; | ||
|
|
||
| // Ensure properties adhere to the Person interface | ||
| return { name, age, occupation }; | ||
| } | ||
|
|
||
| // Usage: | ||
| const person1 = createPerson({ name: 'Alice', age: 30 }); // { name: "Alice", age: 30, occupation: "Unknown" } | ||
| const person2 = createPerson({ name: 'Bob', age: 25, occupation: 'Developer' }); // { name: "Bob", age: 25, occupation: "Developer" } | ||
| ``` | ||
|
|
||
| TypeScript is planning to introduce the `satisfies` keyword, which will provide a more flexible way to declare that a data structure conforms to a specific type without sacrificing readability or maintainability. However, this feature is not yet available. |
There was a problem hiding this comment.
Heads-up that I'll be integrating this section into https://github.com/MetaMask/contributor-docs/pull/74/files#diff-ae03a9f2eb638a25c1d8d7ba853c78220f1ed44a19ad97a8d780cbc0c00a4260R7-R159.
Previous comment on expected changes: https://github.com/MetaMask/contributor-docs/pull/80/files#r1506360422.
| #### `implements` keyword | ||
|
|
||
| The `implements` keyword enables us to define and enforce interfaces, i.e. strict contracts consisting of expected object and class properties and abstract method signatures. | ||
| Writing an interface to establish the specifications of a class that external code can interact while without being aware of internal implementation details is encouraged as sound OOP development practice. |
There was a problem hiding this comment.
Nit: phrasing, grammar
| Writing an interface to establish the specifications of a class that external code can interact while without being aware of internal implementation details is encouraged as sound OOP development practice. | |
| We encourage writing interfaces to establish the specifications of a class. This is sound OOP development practice, as it enables external code to interact with the class without requiring awareness of internal implementation details. |
|
|
||
| #### Don't use numeric enums | ||
|
|
||
| Numeric enums are misleading because it creates a reverse mapping from value to property name, and when using `Object.values` to access member names, it will return the numerical values instead of the member names, potentially causing unexpected behavior. |
There was a problem hiding this comment.
TODO for future iterations:
What unexpected behavior? Why is this practice problematic? Do we need an example for what "reverse mapping" means and why it causes confusion?
The correct example is ambiguous because the corresponding keys and values are identical. It's also unclear why it's better than the incorrect example.
…" or "typescript"
|
@MajorLift thanks for your review and approval, they are really helpful ! I'll draft another PR to address the rest |
|
Have a conversation with @MajorLift offline to merge his work first, then add more for that he will leave for me |
Provide basic guideline for typescript