TypeScript Guidelines#21050
Conversation
|
@allroundexperts Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Comments addressed! |
neil-marcellini
left a comment
There was a problem hiding this comment.
Looking good, I've reviewed all of it now. I have a few small changes I would like to see and a handful of questions. I'm really glad we have you guys working on this, I've learned so much already!
| } | ||
| ``` | ||
|
|
||
| The `any` type allows assignment to all types and dereference of any property, which is undesirable and should be avoided. Instead, and in most cases, use the `unknown` type which expresses a similar concept and is much safer as it requires narrowing the type before using it. When you know that the type structure is a object but you don't have context about the content, use `Record<string, unknown>`. |
There was a problem hiding this comment.
The "string" key can also be a number or a symbol right? We have a lot of objects keyed by integers. I was wondering about that and found the answer on Reddit, but I would love to have a link to a more official source.
There was a problem hiding this comment.
Thanks for finding it in the docs! NAB: Maybe add a sentence to clarify that the Record key can also be a number or a symbol when it's written like this, and link to the TS docs.
There was a problem hiding this comment.
Added clarification to this section @neil-marcellini @fabioh8010.
Maybe add a sentence to clarify that the Record key can also be a number or a symbol
Turns out that Record<string, T> allows numeric keys while not permitting symbol keys: TS playground
|
still reviewing... |
|
Addressed more comments! |
neil-marcellini
left a comment
There was a problem hiding this comment.
I'm happy with it! Thanks.
|
was reviwng TS declaration PR for still reviewing the guideline |
|
still reviewing... I have several items I think we should add to the guideline, but I don't have time to get down on them right now |
|
|
||
| ### Platform-Specific Variants | ||
|
|
||
| In most cases, the code written for this repo should be platform-independent. In such cases, each module should have a single file, `index.ts`, which defines the module's exports. There are, however, some cases in which a feature is intrinsically tied to the underlying platform. In such cases, the following file extensions can be used to export platform-specific code from a module: |
There was a problem hiding this comment.
Let's de-dupe a bit by replace lines 23-31 with the following:
Platform-specific TypeScript files follow the same naming conventions as [JavaScript](https://github.com/Expensify/App#platform-specific-file-extensions) files, except with the `.ts`/`.tsx` extension instead of `.js`.
| export type { ComponentProps, Helpers }; | ||
| ``` | ||
|
|
||
| In case there is no default implementation, you have to create a `index.d.ts` declaration file. When importing the module from other files, TypeScript will automatically pick up the type definitions from `index.d.ts`. Be careful when defining `index.d.ts` as declaration files aren't checked by the TypeScript compiler (with `skipLibCheck: true` - [source](https://www.typescriptlang.org/tsconfig#skipLibCheck)). |
There was a problem hiding this comment.
Apologies if this has been asked-and-answered, but I wonder if, instead of creating two patterns where one of them causes us to lose type-safety, we should instead enforce that every module has a default implementation (i.e: if you have index.desktop.js, index.android.js, and index.ios.js, then we would have index.js and know that it must be for web)
There was a problem hiding this comment.
Here is a long discussion in TS config doc regarding this topic.
TL;DR:
We extracted two types of platform specific variants:
- With default implementation, only when it makes sense to have it (usually applies to more than one platform)
- No default implementation, each platform is different (like in
getPlatformutility - example).
@fabioh8010 comment on platform specific variants without default implementation 👇
I think we don't have better options than using
.d.tsfiles for this kind of situation. ThegetPlatformis a perfect example why I need this type of file because we don't haveindex.tsfor it. An possible alternative would be to choose one of the platform-specific files and rename it toindex.ts, but this is terrible in my opinion. Using.d.tswill be very beneficial for us.
There was a problem hiding this comment.
instead of creating two patterns where one of them causes us to lose type-safety, we should instead enforce that every module has a default implementation
We don't really lose type-safety with declaration files as long as the name of the function is the same and types are shared from types.ts file and used in all implementations.
if you have index.desktop.js, index.android.js, and index.ios.js, then we would have index.js and know that it must be for web
I have two problems with this approach:
- It doesn't really eliminate type-safety problem that you mentioned with declaration files. What if each platform has a return type of literal strings: 'desktop' for desktop, 'ios' for iOS, 'android' for Android and 'web' for web. Rename
index.web.tstoindex.ts, now the return type of default implementation is 'web'. When importing, type 'web' is inferred, but it should be eitherstringor'ios' | 'android' | 'desktop' | 'web'in reality. That's why we came up withtypes.tsfile which exports shared types (Props, return types and whatever is needed) to default and platform specific implementations. - It is less readable in my opinion. In order to know what is the purpose of default implementation, we have to check all remaining files to check what platform uses it.
There was a problem hiding this comment.
If I were to standardize one approach, I would opt for always requiring implementations for each of the four platforms and including an index.d.ts file with a common interface + types.ts with shared types.
The approach described in the doc is also fine with me 👍
There was a problem hiding this comment.
I agree with @blazejkustra that the approach described in the current guideline looks fine.
But if we really want to standardize it, I'll opt for the approach @gedu mentioned: always use index.ts for only export purposes.
// Button.types.ts (types)
export type ButtonProps = {...};
// Button.ios.ts (platform-specific implementation)
import { ButtonProps } from "./Button.types";
export default function Button({...}: ButtonProps) {...};
// Button.ts (default implementation)
import { ButtonProps } from "./Button.types";
export default function Button({...}: ButtonProps) {...};
// index.ts (exports)
export { default } from "./Button";
export * from "./Button.types";This approach is what I see in component libraries and is what I believe is the standard.
What do you think?
cc: @fabioh8010 @blazejkustra
There was a problem hiding this comment.
But if we really want to standardize it, I'll opt for the approach @gedu mentioned: always use index.ts for only export purposes.
What if there is no default implementation? Do we need to implement Button.d.ts file then? I think files structure is less readable with this approach.
This approach is what I see in component libraries and is what I believe is the standard.
Could you provide an example? @hayata-suenaga
There was a problem hiding this comment.
index.ts is usually used to export stuff in component libraries
Those libraries are natively typed and also have Button.types.ts for Button component, for example.
However...
What if there is no default implementation? Do we need to implement Button.d.ts file then?
I tested this, and no, it didn't work without default implementation (without Button.ts) 😢 and creating Buttno.d.ts and index.ts seems too much.
So let's go with our original plan 👍
|
|
||
| Note that `index.ts` should contain the default implementation, and only platform-specific implementations should be done in their respective files. i.e: If you have mobile-specific implementation in `index.native.ts`, then the desktop/web implementation can be contained in a shared `index.ts`. | ||
|
|
||
| For each platform-specific module, create shared type definitions in a separate `types.ts` file and place it in the same folder. `types.ts` has to export shared types which are **compatible with all platform-specific implementations**. Declare component props, return types, and other common types in this file. |
There was a problem hiding this comment.
Will tsc validate that each platform-specific implementation is compatible with the shared types? i.e: it needs to check types not only for index.ts, but also index.native.ts. Will tsc handle that for us?
There was a problem hiding this comment.
Will tsc validate that each platform-specific implementation is compatible with the shared types?
Shared types (types defined in types.ts) will be used in each variant. Each TS file is checked individually, meaning that if a shared type isn't compatible with implementation we will get an error (error will be showed in platform specific implementations, not in types.ts file).
Will tsc validate that each platform-specific implementation is compatible with other platform specific implementations?
No, from TS perspective all these files are separate modules that aren't connected, that means there might be a case where index.ios.ts exports function named 'x' and index.android.ts exports function named 'xx' and tsc won't complain.
There was a problem hiding this comment.
No, from TS perspective all these files are separate modules that aren't connected, that means there might be a case where index.ios.ts exports function named 'x' and index.android.ts exports function named 'xx' and tsc won't complain.
@roryabraham This is the reason we have to define shared types in types.ts and make sure you export them in each platform-specific file.
|
|
||
| Refer to the React Native documentation for more information about [Platform Specific Code](https://reactnative.dev/docs/platform-specific-code). | ||
|
|
||
| ## Naming Conventions |
There was a problem hiding this comment.
It seems like most of this might be possible to enforce via https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/naming-convention.md, maybe we can implement that in #20179 and then remove this section. In general I don't think the guidelines would be needed if they were enforced by lint rules.
There was a problem hiding this comment.
I think this eslint rule is a nice addition to this section. Even with the rule I would leave this part in the guideline.
It seems like most of this might be possible to enforce via link
I don't think it is possible to enforce CONSTANT_CASE to only work for global-level constant values. Other should be fine I believe 👍
There was a problem hiding this comment.
I never used this rule before. I'm not sure if we need it as I haven't seen any naming convention discrepancies in the codebase but it is nice addition and I can add it to the Typescript setup PR!
@hayata-suenaga @fabioh8010 thoughts?
There was a problem hiding this comment.
@roryabraham, great find! @blazejkustra do you think you can figure out options for this rule to make the rule conform to our naming standard?
I think it's worth writing down even short sentences in the guideline even lint rules enforce some aspects of the guideline.
otherwise, we also don't need section for Enum or any or the usage of @ts-ignore
There was a problem hiding this comment.
@blazejkustra I think you last commit about the eslint rule solve this discussion as well, isn't?
There was a problem hiding this comment.
I tried and came up with this rule (added it to the Typescript setup PR):
'@typescript-eslint/naming-convention': [
'error',
{
selector: 'variable',
format: ['camelCase', 'UPPER_CASE', 'PascalCase'],
},
{
selector: ['property', 'function'],
format: ['camelCase', 'PascalCase'],
},
{
selector: ['enumMember', 'typeLike', 'typeParameter'],
format: ['PascalCase'],
},
{
selector: ['parameter', 'method'],
format: ['camelCase'],
},
],Explanations:
variable:camelCasefor basic variables,UPPER_CASEfor global-level constants,PascalCasefor React Components (we need it when usingforwardRef)property,function: 'camelCase' for default functions and object properties.PascalCasefor react components and Enum like object properties.enumMember,typeLike,typeParameter:PascalCasefor all types.parameter,method:camelCasefor class methods and function parameters
This rule doesn't fully conform with our naming standards:
- Global-level constants can be written with
camelCaseorPascalCase. - Object properties can be written in
PascalCase. - Functions (not components) can be written in
PascalCase.
There are more inconsistencies like this due to the fact that some selectors are used in multiple contexts. It doesn't fully conform with our naming standards, it's still helpful and eliminates some of the mistakes that could happen.
Thoughts @fabioh8010 @hayata-suenaga @roryabraham?
There was a problem hiding this comment.
LGTM! In this case, we should keep the conventions in the guideline as it's not possible to cover all cases with the rule.
There was a problem hiding this comment.
We should put non-typescript naming conventions inside our existing JavaScript style guidelines.
I know linting of the naming convention was made possible because we introduced TS eslint plugin, but let's keep JavaScript stuff in STYLE.md instead of TypeScript guideline
|
|
||
| The `any` type allows assignment to all types and dereference of any property, which is undesirable and should be avoided. Instead, and in most cases, use the `unknown` type which expresses a similar concept and is much safer as it requires narrowing the type before using it. | ||
|
|
||
| When you know that the type structure is a object but you don't have context about the content, use `Record<string, unknown>`. Note that numeric keys are allowed with the `Record<string, unknown>` type. Numeric keys are implicitly converted to strings during property assignment and access. To read more about this, see [TypeScript documentation](https://www.typescriptlang.org/docs/handbook/2/objects.html#dynamically-adding-properties). |
There was a problem hiding this comment.
| When you know that the type structure is a object but you don't have context about the content, use `Record<string, unknown>`. Note that numeric keys are allowed with the `Record<string, unknown>` type. Numeric keys are implicitly converted to strings during property assignment and access. To read more about this, see [TypeScript documentation](https://www.typescriptlang.org/docs/handbook/2/objects.html#dynamically-adding-properties). | |
| When you know that the type structure is an object, but you don't have context about the content, use `Record<string, unknown>`. Note that numeric keys are allowed with the `Record<string, unknown>` type. Numeric keys are implicitly converted to strings during property assignment and access. To read more about this, see [TypeScript documentation](https://www.typescriptlang.org/docs/handbook/2/objects.html#dynamically-adding-properties). |
There was a problem hiding this comment.
I also think it might be good to include an example showing Record<string, unknown>, because that's not included in the example that follows.
There was a problem hiding this comment.
Changed the text and added an example.
|
|
||
| To achieve better and safer typing of components with default prop values, all usages `defaultProps` shall be removed and replaced by prop destructuring. Please head to **Typing components** section to understand how you can convert your implementation. | ||
|
|
||
| ## JSDoc annotations |
There was a problem hiding this comment.
Can we also enforce this with a lint rule?
There was a problem hiding this comment.
I found this rule to disallow types on @returns and @param tags - link. I did not find a rule to completely ban params and returns 😒
@hayata-suenaga @fabioh8010 wdyt?
There was a problem hiding this comment.
@blazejkustra thank you for the research 🙌
Let's use the plugin and specify the rule for not using type annotations 👍
|
I reviewed the guideline over the weekend. I like the guideline and great job @fabioh8010 🍾 I just wanted to organize the guideline more and wanted to make it way more concise. So I created another PR based off this guideline PR. You can also see how the markdown files look like in this repo I created. Please review that one when you have time this week. (I'm still finalizing details and still doing proofreading for grammatical and spelling mistakes). In the new PR, I split this guideline into two files (one contains rules that must be followed and the other one contains guides on how to accomplish certain things in TypeScript (ex. typing Most of the stuff is just copy and past from this PP, but I drastically cut off guidelines that are already covered by ESLint rules. For those rules that errors if not followed by ESLint, I made the wording more strict ex. "Do not use Also, I added instructions on what to do when rules in guidelines cannot be followed for certain situation (posting comments on @fabioh8010 @blazejkustra @mrousavy I couldn't add you to the new PR as reviewers. Let we know if you cannot access it. |
|
|
||
| // index.website.ts | ||
| export default function getPlatform(): Platform { | ||
| return "website"; |
|
|
||
| Refer to the React Native documentation for more information about [Platform Specific Code](https://reactnative.dev/docs/platform-specific-code). | ||
|
|
||
| ## Naming Conventions |
There was a problem hiding this comment.
I tried and came up with this rule (added it to the Typescript setup PR):
'@typescript-eslint/naming-convention': [
'error',
{
selector: 'variable',
format: ['camelCase', 'UPPER_CASE', 'PascalCase'],
},
{
selector: ['property', 'function'],
format: ['camelCase', 'PascalCase'],
},
{
selector: ['enumMember', 'typeLike', 'typeParameter'],
format: ['PascalCase'],
},
{
selector: ['parameter', 'method'],
format: ['camelCase'],
},
],Explanations:
variable:camelCasefor basic variables,UPPER_CASEfor global-level constants,PascalCasefor React Components (we need it when usingforwardRef)property,function: 'camelCase' for default functions and object properties.PascalCasefor react components and Enum like object properties.enumMember,typeLike,typeParameter:PascalCasefor all types.parameter,method:camelCasefor class methods and function parameters
This rule doesn't fully conform with our naming standards:
- Global-level constants can be written with
camelCaseorPascalCase. - Object properties can be written in
PascalCase. - Functions (not components) can be written in
PascalCase.
There are more inconsistencies like this due to the fact that some selectors are used in multiple contexts. It doesn't fully conform with our naming standards, it's still helpful and eliminates some of the mistakes that could happen.
Thoughts @fabioh8010 @hayata-suenaga @roryabraham?
|
Closing this one in favor of the new PR 🙇 We can keep existing unresolved conversations happening here |
Details
This PR aims to establish the TypeScript guidelines that are going to be used in the project.
Fixed Issues
$ #20623
PROPOSAL: -
Tests
Not applicable.
Offline tests
Not applicable.
QA Steps
Not applicable.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Not applicable.