From 71afddffaa5d839b13a34491275e89d941eed2b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Thu, 14 Dec 2023 09:05:38 +0000 Subject: [PATCH 1/6] Add instructions for bypassing blocking TS issues --- contributingGuides/TS_STYLE.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/contributingGuides/TS_STYLE.md b/contributingGuides/TS_STYLE.md index bc62020ffd54..610b00fbdafb 100644 --- a/contributingGuides/TS_STYLE.md +++ b/contributingGuides/TS_STYLE.md @@ -579,6 +579,25 @@ object?.foo ?? 'bar'; const y: number = 123; // TS error: Unused '@ts-expect-error' directive. ``` +- The TS issue I'm working on is blocked by another TS issue because of type errors. What should I do? + + In order to proceed with the migration faster, we are now allowing the use of `@ts-expect-error` annotation to temporally suppress those errors and help you unblock your issues. The only requirements is that you MUST add the annotation with a comment explaining that it must be removed when the blocking issue is migrated, e.g.: + + ```ts + return ( + + ); + ``` + + **You will also need to reference the blocking issue in your PR.** You can find all the TS issues [here](https://github.com/orgs/Expensify/projects/46). + ## Learning Resources ### Quickest way to learn TypeScript From 65d7d36f5e24c14e2a033a9d2299b5b7f043b2c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Thu, 14 Dec 2023 10:19:55 +0000 Subject: [PATCH 2/6] Add instructions about removal of compose and usage of hooks instead of hocs --- contributingGuides/TS_STYLE.md | 89 +++++++++++++++++++++++++++++++++- 1 file changed, 88 insertions(+), 1 deletion(-) diff --git a/contributingGuides/TS_STYLE.md b/contributingGuides/TS_STYLE.md index 610b00fbdafb..7973150cd4a8 100644 --- a/contributingGuides/TS_STYLE.md +++ b/contributingGuides/TS_STYLE.md @@ -24,6 +24,8 @@ - [1.17 `.tsx`](#tsx) - [1.18 No inline prop types](#no-inline-prop-types) - [1.19 Satisfies operator](#satisfies-operator) + - [1.20 Hooks instead of HOCs](#hooks-instead-of-hocs) + - [1.21 `compose` usage](#compose-usage) - [Exception to Rules](#exception-to-rules) - [Communication Items](#communication-items) - [Migration Guidelines](#migration-guidelines) @@ -509,6 +511,91 @@ type Foo = { } satisfies Record; ``` + + +- [1.20](#hooks-instead-of-hocs) **Hooks instead of HOCs**: Replace HOCs usage with Hooks whenever possible. + + > Why? Hooks are easier to use (can be used inside the function component), and don't need nesting or `compose` when exporting the component. It also allows us to remove `compose` completely in some components since it has been bringing up some issues with TypeScript. Read the [`compose` usage](#compose-usage) section for further information about the TypeScript issues with `compose`. + + ```tsx + // BAD + type ComponentOnyxProps = { + session: OnyxEntry; + }; + + type ComponentProps = WindowDimensionsProps & + WithLocalizeProps & + ComponentOnyxProps & { + someProp: string; + }; + + function Component({windowWidth, windowHeight, translate, session, someProp}: ComponentProps) { + // component's code + } + + export default compose( + withWindowDimensions, + withLocalize, + withOnyx({ + session: { + key: ONYXKEYS.SESSION, + }, + }), + )(Component); + + // GOOD + type ComponentOnyxProps = { + session: OnyxEntry; + }; + + type ComponentProps = ComponentOnyxProps & { + someProp: string; + }; + + function Component({session, someProp}: ComponentProps) { + const {windowWidth, windowHeight} = useWindowDimensions(); + const {translate} = useLocalize(); + // component's code + } + + // There is no hook alternative for withOnyx yet. + export default withOnyx({ + session: { + key: ONYXKEYS.SESSION, + }, + })(Component); + ``` + + + +- [1.21](#compose-usage) **`compose` usage**: Avoid the usage of `compose` function to compose HOCs in TypeScript files. Use nesting instead. + + > Why? `compose` function doesn't work well with TypeScript when dealing with several HOCs being used in a component, many times resulting in wrong types and errors. Instead, nesting can be used to allow a seamless use of multiple HOCs and result in a correct return type of the compoment. Also, you can use [hooks instead of HOCs](#hooks-instead-of-hocs) whenever possible to minimize or even remove the need of HOCs in the component. + + ```ts + // BAD + export default compose( + withWindowDimensions, + withLocalize, + withOnyx({ + session: { + key: ONYXKEYS.SESSION, + }, + }), + )(Component); + + // GOOD + export default withWindowDimensions( + withLocalize( + withOnyx({ + session: { + key: ONYXKEYS.SESSION, + }, + })(Component), + ), + ); + ``` + ## Exception to Rules Most of the rules are enforced in ESLint or checked by TypeScript. If you think your particular situation warrants an exception, post the context in the `#expensify-open-source` Slack channel with your message prefixed with `TS EXCEPTION:`. The internal engineer assigned to the PR should be the one that approves each exception, however all discussion regarding granting exceptions should happen in the public channel instead of the GitHub PR page so that the TS migration team can access them easily. @@ -583,7 +670,7 @@ object?.foo ?? 'bar'; In order to proceed with the migration faster, we are now allowing the use of `@ts-expect-error` annotation to temporally suppress those errors and help you unblock your issues. The only requirements is that you MUST add the annotation with a comment explaining that it must be removed when the blocking issue is migrated, e.g.: - ```ts + ```tsx return ( Date: Thu, 14 Dec 2023 10:22:55 +0000 Subject: [PATCH 3/6] Update module augmentation instructions to mention internal JavsScript modules too --- contributingGuides/TS_STYLE.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contributingGuides/TS_STYLE.md b/contributingGuides/TS_STYLE.md index 7973150cd4a8..9a3d33014b54 100644 --- a/contributingGuides/TS_STYLE.md +++ b/contributingGuides/TS_STYLE.md @@ -126,7 +126,7 @@ type Foo = { -- [1.2](#d-ts-extension) **`d.ts` Extension**: Do not use `d.ts` file extension even when a file contains only type declarations. Only exceptions are `src/types/global.d.ts` and `src/types/modules/*.d.ts` files in which third party packages can be modified using module augmentation. Refer to the [Communication Items](#communication-items) section to learn more about module augmentation. +- [1.2](#d-ts-extension) **`d.ts` Extension**: Do not use `d.ts` file extension even when a file contains only type declarations. Only exceptions are `src/types/global.d.ts` and `src/types/modules/*.d.ts` files in which third party packages and internal JavaScript modules can be modified using module augmentation. Refer to the [Communication Items](#communication-items) section to learn more about module augmentation. > Why? Type errors in `d.ts` files are not checked by TypeScript [^1]. @@ -608,7 +608,7 @@ This rule will apply until the migration is done. After the migration, discussio > Comment in the `#expensify-open-source` Slack channel if any of the following situations are encountered. Each comment should be prefixed with `TS ATTENTION:`. Internal engineers will access each situation and prescribe solutions to each case. Internal engineers should refer to general solutions to each situation that follows each list item. -- I think types definitions in a third party library is incomplete or incorrect +- I think types definitions in a third party library or internal JavaScript module are incomplete or incorrect When the library indeed contains incorrect or missing type definitions and it cannot be updated, use module augmentation to correct them. All module augmentation code should be contained in `/src/types/modules/*.d.ts`, each library as a separate file. From 0e94be399d62113ec385b8456a2376a8c5da32a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Thu, 14 Dec 2023 10:26:08 +0000 Subject: [PATCH 4/6] Update guidelines about new TS code --- contributingGuides/TS_STYLE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contributingGuides/TS_STYLE.md b/contributingGuides/TS_STYLE.md index 9a3d33014b54..6955a330d97d 100644 --- a/contributingGuides/TS_STYLE.md +++ b/contributingGuides/TS_STYLE.md @@ -627,7 +627,7 @@ declare module "external-library-name" { > This section contains instructions that are applicable during the migration. -- 🚨 DO NOT write new code in TypeScript yet. The only time you write TypeScript code is when the file you're editing has already been migrated to TypeScript by the migration team. This guideline will be updated once it's time for new code to be written in TypeScript. If you're doing a major overhaul or refactoring of particular features or utilities of App and you believe it might be beneficial to migrate relevant code to TypeScript as part of the refactoring, please ask in the #expensify-open-source channel about it (and prefix your message with `TS ATTENTION:`). +- 🚨 DO NOT write new code in TypeScript yet. The only time you write TypeScript code is when the file you're editing has already been migrated to TypeScript by the migration team, or when you need to add new files under `src/libs`, `src/hooks`, `src/styles`, and `src/languages` directories. This guideline will be updated once it's time for new code to be written in TypeScript. If you're doing a major overhaul or refactoring of particular features or utilities of App and you believe it might be beneficial to migrate relevant code to TypeScript as part of the refactoring, please ask in the #expensify-open-source channel about it (and prefix your message with `TS ATTENTION:`). - If you're migrating a module that doesn't have a default implementation (i.e. `index.ts`, e.g. `getPlatform`), convert `index.website.js` to `index.ts`. Without `index.ts`, TypeScript cannot get type information where the module is imported. From 9a4383d45d6bca62b019d7ea5a5544b46abdd776 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Mon, 18 Dec 2023 11:43:33 +0000 Subject: [PATCH 5/6] Address review comments --- contributingGuides/TS_STYLE.md | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/contributingGuides/TS_STYLE.md b/contributingGuides/TS_STYLE.md index 6955a330d97d..24078b94064e 100644 --- a/contributingGuides/TS_STYLE.md +++ b/contributingGuides/TS_STYLE.md @@ -126,7 +126,7 @@ type Foo = { -- [1.2](#d-ts-extension) **`d.ts` Extension**: Do not use `d.ts` file extension even when a file contains only type declarations. Only exceptions are `src/types/global.d.ts` and `src/types/modules/*.d.ts` files in which third party packages and internal JavaScript modules can be modified using module augmentation. Refer to the [Communication Items](#communication-items) section to learn more about module augmentation. +- [1.2](#d-ts-extension) **`d.ts` Extension**: Do not use `d.ts` file extension even when a file contains only type declarations. Only exceptions are `src/types/global.d.ts` and `src/types/modules/*.d.ts` files in which third party packages and JavaScript's built-in modules (e.g. `window` object) can be modified using module augmentation. Refer to the [Communication Items](#communication-items) section to learn more about module augmentation. > Why? Type errors in `d.ts` files are not checked by TypeScript [^1]. @@ -517,6 +517,8 @@ type Foo = { > Why? Hooks are easier to use (can be used inside the function component), and don't need nesting or `compose` when exporting the component. It also allows us to remove `compose` completely in some components since it has been bringing up some issues with TypeScript. Read the [`compose` usage](#compose-usage) section for further information about the TypeScript issues with `compose`. + > Note: Because Onyx doesn't provide a hook yet, in a component that accesses Onyx data with `withOnyx` HOC, please make sure that you don't use other HOCs (if applicable) to avoid HOC nesting. + ```tsx // BAD type ComponentOnyxProps = { @@ -575,8 +577,8 @@ type Foo = { ```ts // BAD export default compose( - withWindowDimensions, - withLocalize, + withCurrentUserPersonalDetails, + withReportOrNotFound(), withOnyx({ session: { key: ONYXKEYS.SESSION, @@ -585,8 +587,8 @@ type Foo = { )(Component); // GOOD - export default withWindowDimensions( - withLocalize( + export default withCurrentUserPersonalDetails( + withReportOrNotFound()( withOnyx({ session: { key: ONYXKEYS.SESSION, @@ -594,6 +596,15 @@ type Foo = { })(Component), ), ); + + // GOOD - alternative to HOC nesting + const ComponentWithOnyx = withOnyx({ + session: { + key: ONYXKEYS.SESSION, + }, + })(Component); + const ComponentWithReportOrNotFound = withReportOrNotFound()(ComponentWithOnyx); + export default withCurrentUserPersonalDetails(ComponentWithReportOrNotFound); ``` ## Exception to Rules @@ -608,7 +619,7 @@ This rule will apply until the migration is done. After the migration, discussio > Comment in the `#expensify-open-source` Slack channel if any of the following situations are encountered. Each comment should be prefixed with `TS ATTENTION:`. Internal engineers will access each situation and prescribe solutions to each case. Internal engineers should refer to general solutions to each situation that follows each list item. -- I think types definitions in a third party library or internal JavaScript module are incomplete or incorrect +- I think types definitions in a third party library or JavaScript's built-in module are incomplete or incorrect When the library indeed contains incorrect or missing type definitions and it cannot be updated, use module augmentation to correct them. All module augmentation code should be contained in `/src/types/modules/*.d.ts`, each library as a separate file. From 1505e457b68241cbfe8786012bec0a53fbb448d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Henriques?= Date: Mon, 18 Dec 2023 16:18:36 +0000 Subject: [PATCH 6/6] Fix indentation --- contributingGuides/TS_STYLE.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/contributingGuides/TS_STYLE.md b/contributingGuides/TS_STYLE.md index 24078b94064e..a583941bf71d 100644 --- a/contributingGuides/TS_STYLE.md +++ b/contributingGuides/TS_STYLE.md @@ -599,12 +599,12 @@ type Foo = { // GOOD - alternative to HOC nesting const ComponentWithOnyx = withOnyx({ - session: { - key: ONYXKEYS.SESSION, - }, - })(Component); - const ComponentWithReportOrNotFound = withReportOrNotFound()(ComponentWithOnyx); - export default withCurrentUserPersonalDetails(ComponentWithReportOrNotFound); + session: { + key: ONYXKEYS.SESSION, + }, + })(Component); + const ComponentWithReportOrNotFound = withReportOrNotFound()(ComponentWithOnyx); + export default withCurrentUserPersonalDetails(ComponentWithReportOrNotFound); ``` ## Exception to Rules