[No QA] [TS Migration] Adjust guidelines, both TS and JS markdown files#41212
Merged
roryabraham merged 8 commits intoExpensify:mainfrom May 10, 2024
Merged
[No QA] [TS Migration] Adjust guidelines, both TS and JS markdown files#41212roryabraham merged 8 commits intoExpensify:mainfrom
roryabraham merged 8 commits intoExpensify:mainfrom
Conversation
Contributor
Author
|
Thanks for reviewing @blazejkustra! Changes applied 😄 |
blazejkustra
approved these changes
May 7, 2024
Contributor
|
Let's tag some Internal people too, they might want to read this through 😄 |
roryabraham
requested changes
May 8, 2024
contributingGuides/STYLE.md
Outdated
|
|
||
| ### `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. |
Contributor
There was a problem hiding this comment.
Suggested change
| 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. | |
| 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](https://www.typescriptlang.org/docs/handbook/declaration-merging.html#module-augmentation). |
contributingGuides/STYLE.md
Outdated
|
|
||
| ### Prop Types | ||
|
|
||
| Don't use `ComponentProps` to grab a component's prop types. Go to the source file for the component and export prop types from there. Import and use the exported prop types. |
Contributor
There was a problem hiding this comment.
I'm actually not sure why we decided this, so maybe we should add a why clause here?
Contributor
Author
There was a problem hiding this comment.
I'm not 100% sure either, as it was copied from TS_STYLE.md. I think it might be mostly for consistency, I have also seen some threads with people complaining about ComponentProps not always working, e.g. for class components with default props. Any ideas what we should put in the why section?
Contributor
There was a problem hiding this comment.
I don't remember the exact reason why we added this rule, I think it is because of couple reasons:
- Importing props from the component file is very natural and more popular overall.
- I find importing props more readable == personal preference
- Turns out there are some gotchas when using this prop, so it is less reliable
Contributor
There was a problem hiding this comment.
Yes, since we have our own props defined in the codebase is always easy to just export and use it when required. ComponentProps (and its friends ComponentPropsWithoutRef, ComponentPropsWithRef) should be used only for specific situations or generic components.
Contributor
Author
There was a problem hiding this comment.
Thanks guys, changes applied!
contributingGuides/STYLE.md
Outdated
|
|
||
| In modules with platform-specific implementations, create `types.ts` to define shared types. Import and use shared types in each platform specific files. Do not use [`satisfies` operator](#satisfies-operator) for platform-specific implementations, always define shared types that complies with all variants. | ||
|
|
||
| > Why? To encourage consistent API across platform-specific implementations. If you're migrating module that doesn't have a default implement (i.e. `index.ts`, e.g. `getPlatform`), refer to [Migration Guidelines](#migration-guidelines) for further information. |
Contributor
There was a problem hiding this comment.
Suggested change
| > Why? To encourage consistent API across platform-specific implementations. If you're migrating module that doesn't have a default implement (i.e. `index.ts`, e.g. `getPlatform`), refer to [Migration Guidelines](#migration-guidelines) for further information. | |
| > Why? To encourage consistent API across platform-specific implementations. If you're migrating module that doesn't have a default implementation (i.e. `index.ts`, e.g. `getPlatform`), refer to [Migration Guidelines](#migration-guidelines) for further information. |
contributingGuides/STYLE.md
Outdated
| mw100: { | ||
| maxWidth: '100%', | ||
| }, | ||
| } satisfies Record<string, ViewStyle>; |
Contributor
There was a problem hiding this comment.
question: Would you need to do this to get the most narrow type?
Suggested change
| } satisfies Record<string, ViewStyle>; | |
| } as const satisfies Record<string, ViewStyle>; |
Contributor
Author
There was a problem hiding this comment.
I think so, but @fabioh8010 @blazejkustra can you confirm?
Contributor
There was a problem hiding this comment.
question: Would you need to do this to get the most narrow type?
Yes that is correct, we could put another example as there are cases as const satisfies is not desirable.
Contributor
There was a problem hiding this comment.
I think it makes a difference in some cases, example:
satisfies Record<string, ViewStyle>:
br0: {
borderRadius: number;
};
br1: {
borderRadius: number;
};
}as const satisfies Record<string, ViewStyle>:
{
readonly br0: {
readonly borderRadius: 0;
};
readonly br1: {
readonly borderRadius: 4;
};
}So if we see any value in such narrow type I'm good to adjust the guidelines 👍
contributingGuides/STYLE.md
Outdated
|
|
||
| > 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. |
Contributor
There was a problem hiding this comment.
This is out of date - let's update this to formalize useOnyx as a best practice over withOnyx
| } | ||
|
|
||
| // There is no hook alternative for withOnyx yet. | ||
| export default withOnyx<ComponentProps, ComponentOnyxProps>({ |
Contributor
There was a problem hiding this comment.
Let's update this for useOnyx too
contributingGuides/STYLE.md
Outdated
|
|
||
| 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. |
Contributor
There was a problem hiding this comment.
Suggested change
| > 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. | |
| > 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. |
roryabraham
requested changes
May 8, 2024
| }; | ||
|
|
||
| // GOOD | ||
| type ReadOnlyFoo = Readonly<Foo>; |
Contributor
There was a problem hiding this comment.
Can we please also add an example for ValueOf?
// BAD
type FooValue = Foo[keyof Foo];
// GOOD
type FooValue = ValueOf<Foo>;
Contributor
Author
|
@roryabraham @blazejkustra @fabioh8010 PR updated, feel free to take another look! |
roryabraham
approved these changes
May 10, 2024
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
74 tasks
Contributor
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.4.73-0 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.73-7 🚀
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
Fixed Issues
$ #39118
PROPOSAL: N/A
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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.