[No QA] [TS Migration] Adjust guidelines, both TS and JS markdown files #41212
Conversation
This file contains 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
|
Thanks for reviewing @blazejkustra! Changes applied 😄 |
blazejkustra
approved these changes
May 7, 2024
|
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. |
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. |
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?
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?
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
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.
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. |
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>; |
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>; |
There was a problem hiding this comment.
I think so, but @fabioh8010 @blazejkustra can you confirm?
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.
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. |
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>({ |
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. |
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>; |
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>;
|
@roryabraham @blazejkustra @fabioh8010 PR updated, feel free to take another look! |
roryabraham
approved these changes
May 10, 2024
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
74 tasks
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.4.73-0 🚀
|
|
🚀 Deployed to production by https://github.com/Beamanator in version: 1.4.73-7 🚀
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.