-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[No QA][TS migration] Update TS guidelines #33054
[No QA][TS migration] Update TS guidelines #33054
Conversation
contributingGuides/TS_STYLE.md
Outdated
@@ -124,7 +126,7 @@ type Foo = { | |||
|
|||
<a name="d-ts-extension"></a><a name="1.2"></a> | |||
|
|||
- [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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suggests that d.ts
can be used for internal modules, but doesn't provide any guidance on when that would be permissible. When would we do this? Can we provide an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you meant that when the internal package written in JavaScript needs to be module-augmented, we use *d.ts
in the modules
directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe let's be more specific or add an example? @fabioh8010
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed internal JavaScript module
to JavaScript's built-in module
, I think makes more clear what we are referring too.
@roryabraham Here is an example of what we are talking about!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabioh8010 I still don't fully understand what you mean by JavaScript's built-in module
. Do you mean functions and constants that are available during run time (those provided by the run time environment)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hayata-suenaga I mean global objects like window
(and its Window
interface), built-in TS interfaces like File
, Event
or any other DOM interfaces available. We have some examples in this file!
@blazejkustra @roryabraham @hayata-suenaga Please have a look again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor thing, other than that LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be nice to clarify this point, but other than that, the change looks good to me 👍
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.4.14-0 🚀
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.4.14-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.4.14-6 🚀
|
Details
This PR makes the following changes in the TypeScript guidelines:
@ts-expect-error
annotation in the cases where we need to unblock an TS issue that is blocked by another one due to type errors.compose
usage because of the issues related to TypeScript.Fixed Issues
$
PROPOSAL:
Tests
N/A
Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop