Check and remove unnecessary @ts-expect-error suppressions#40627
Check and remove unnecessary @ts-expect-error suppressions#40627grgia merged 27 commits intoExpensify:mainfrom
Conversation
|
@Julesssss 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] |
|
Is this done? Melvin did not auto request a review from me because you made the PR ready for review without adding the issue number in the template. If this is ready please complete the checklist. |
Hello But in any case, the PR is about 50 percent ready) I will write to you when the PR is ready for review ) |
|
Hello )
|
I think, I'm missing what rule are we talking about 😄
Please link what are the issues 🤔
These have to stay unfortunately |
These are Github/actions and sorry about the rule inside patches And problem with libraries |
src/Expensify.tsx
Outdated
| type ExpensifyProps = ExpensifyOnyxProps; | ||
|
|
||
| const SplashScreenHiddenContext = React.createContext({}); | ||
| const SplashScreenHiddenContext = React.createContext<{isSplashHidden?: boolean}>({}); |
There was a problem hiding this comment.
Let's extract the type to a separate type
src/libs/ObjectUtils.ts
Outdated
| const keys1 = Object.keys(obj1) as Array<keyof typeof obj1>; | ||
| const keys2 = Object.keys(obj2) as Array<keyof typeof obj2>; |
There was a problem hiding this comment.
Are these assertions necessary?
There was a problem hiding this comment.
If we remove we will have this ts-issue
There was a problem hiding this comment.
Let's improve shallowCompare:
function shallowCompare(obj1?: Record<string, unknown>, obj2?: Record<string, unknown>): boolean {
if (!obj1 && !obj2) {
return true;
}
if (obj1 && obj2) {
const keys1 = Object.keys(obj1);
const keys2 = Object.keys(obj2);
return keys1.length === keys2.length && keys1.every((key) => obj1[key] === obj2[key]);
}
return false;
}
export default shallowCompare;And then you have to make the following adjustments:
diff --git a/src/libs/Navigation/AppNavigator/getPartialStateDiff.ts b/src/libs/Navigation/AppNavigator/getPartialStateDiff.ts
index 4c18e161c9..5061c75007 100644
--- a/src/libs/Navigation/AppNavigator/getPartialStateDiff.ts
+++ b/src/libs/Navigation/AppNavigator/getPartialStateDiff.ts
@@ -53,7 +53,7 @@ function getPartialStateDiff(state: State<RootStackParamList>, templateState: St
(stateTopmostCentralPane &&
templateStateTopmostCentralPane &&
stateTopmostCentralPane.name !== templateStateTopmostCentralPane.name &&
- !shallowCompare(stateTopmostCentralPane.params, templateStateTopmostCentralPane.params))
+ !shallowCompare(stateTopmostCentralPane.params as Record<string, unknown> | undefined, templateStateTopmostCentralPane.params as Record<string, unknown> | undefined))
) {
// We need to wrap central pane routes in the central pane navigator.
diff[NAVIGATORS.CENTRAL_PANE_NAVIGATOR] = templateStateTopmostCentralPane;
@@ -73,7 +73,7 @@ function getPartialStateDiff(state: State<RootStackParamList>, templateState: St
(stateTopmostFullScreen &&
templateStateTopmostFullScreen &&
stateTopmostFullScreen.name !== templateStateTopmostFullScreen.name &&
- !shallowCompare(stateTopmostFullScreen.params, templateStateTopmostFullScreen.params))
+ !shallowCompare(stateTopmostFullScreen.params as Record<string, unknown> | undefined, templateStateTopmostFullScreen.params as Record<string, unknown> | undefined))
) {
diff[NAVIGATORS.FULL_SCREEN_NAVIGATOR] = fullScreenDiff;
}
diff --git a/src/libs/Navigation/linkTo.ts b/src/libs/Navigation/linkTo.ts
index 863cb102ad..b21a24a76e 100644
--- a/src/libs/Navigation/linkTo.ts
+++ b/src/libs/Navigation/linkTo.ts
@@ -153,7 +153,7 @@ export default function linkTo(navigation: NavigationContainerRef<RootStackParam
const isTargetNavigatorOnTop = topRouteName === action.payload.name;
const isTargetScreenDifferentThanCurrent = Boolean(topmostCentralPaneRoute && topmostCentralPaneRoute.name !== action.payload.params?.screen);
- const areParamsDifferent = !shallowCompare(topmostCentralPaneRoute?.params, action.payload.params?.params);
+ const areParamsDifferent = !shallowCompare(topmostCentralPaneRoute?.params as Record<string, unknown> | undefined, action.payload.params?.params);
// In case if type is 'FORCED_UP' we replace current screen with the provided. This means the current screen no longer exists in the stack
if (type === CONST.NAVIGATION.TYPE.FORCED_UP) {
| blur: () => void; | ||
| focus: (shouldDelay?: boolean) => void; | ||
| replaceSelectionWithText: (text: string, shouldAddTrailSpace: boolean) => void; | ||
| replaceSelectionWithText: (text: string, shouldAddTrailSpace: Emoji) => void; |
There was a problem hiding this comment.
How about this?
| replaceSelectionWithText: (text: string, shouldAddTrailSpace: Emoji) => void; | |
| replaceSelectionWithText: OnEmojiSelected; |
| (text: string | undefined) => { | ||
| if (text === undefined) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
We need to test this change 👍
There was a problem hiding this comment.
I removed this changes
Because we can pass only string and undefined in this function is not needed
| onBackButtonPress={() => { | ||
| const backTo = params?.backTo ?? ''; | ||
| let backToRoute = ''; | ||
| let backToRoute = '' as Route; |
There was a problem hiding this comment.
Suggestion:
let backToRoute: Route | undefined;
if (backTo) {
backToRoute = appendParam(backTo, 'state', currentState ?? '');
}
Navigation.goBack(backToRoute);I think we should fix appendParam so that it returns a Route type. This way we can avoid assertion in this file
That's unusual, can you investigate why these // ts-expect-errors are added to JS files in the first place? (you don't need to remove them, but at least let's understand why they exist)
Let's try to fix these library ts expect errors, you can augment types from other libraries like we do in |
|
@ZhenjaHorbach Let me know once PR is ready for review again 😄 |
No problem ) |
|
Is this complete? If it is, please complete the checklist and bump me. Thanks. |
It's not over yet) |
|
@fabioh8010 @blazejkustra I'm working on PR App/src/components/MoneyRequestConfirmationList.tsx Lines 1017 to 1035 in 5f50430 |
|
I can share the current status Don't need to remove Here we are trying to test non-existent methods
Here we are trying to get non-existent this Haven't figured out how to remove rules I tried to remove this rule but as a result we need to change many types inside React-navigation library and I'm not sure how much that makes sense
I tried updating GithubUtils types but it didn't help
I tried updating types using d.ts files but it didn't help. More precisely, the types refer to our files with types, but the error does not disappear |
@ZhenjaHorbach I think all these ones are okay to leave in the codebase, in some cases isn't really possible to remove and that's understandable. |
Sounds great) @c3024 |
|
Please remove |
ZhenjaHorbach
changed the title
I merged main |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
grgia
left a comment
grgia
left a comment
There was a problem hiding this comment.
Seems like there's more changes in this PR besides @ts-expect-error suppressions could you explain in the description?
|
@ZhenjaHorbach could you ping me and @c3024 when you are ready for final review? |
Sorry for delay In other places there are cases when I updated the types in the libs, for example |
And PR is ready In the last commits I reverted changes in IOUTest file and added new changes back So technically I didn't change anything much ) @c3024 |
|
🚀 Deployed to staging by https://github.com/grgia in version: 1.4.75-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.75-1 🚀
|
6 participants
Details
Fixed Issues
$ #39130
PROPOSAL: #39130 (comment)
Tests
Nothing to test. All changes are related to TS only
Offline tests
Nothing to test. All changes are related to TS only
QA Steps
Nothing to test. All changes are related to TS only
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.Screenshots/Videos
Android: Native
NA
Android: mWeb Chrome
NA
iOS: Native
NA
iOS: mWeb Safari
NA
MacOS: Chrome / Safari
NA
MacOS: Desktop
NA