-
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
[TS migration] Migrate 'BankAccounts.js' lib to TypeScript #28316
[TS migration] Migrate 'BankAccounts.js' lib to TypeScript #28316
Conversation
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.
Left a few comments. Overall, LGTM.
@VickyStash Is this PR ready for cross review? |
@blazejkustra The only blocker here is that this PR has OnyxData type declared and your API PR also has it, if it's ok then I can mark it as ready for cross review |
@VickyStash Don't worry, we can clean that in the '[TS migration] Review and clean up Libs files' PR. |
Reviewer Checklist
Screenshots/VideosWeb01_MacOS_Chrome.mp4Mobile Web - Chrome02_Android_Chrome.mp4Mobile Web - Safari03_iOS_Safari.mp4Desktop04_MacOS_Desktop.mp4iOS05_iOS_Native.mp4Android06_Android_Native.mp4 |
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.
LGTM
We did not find an internal engineer to review this PR, trying to assign a random engineer to #24903 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
const commandName = 'ConnectBankAccountWithPlaid'; | ||
|
||
const parameters = { | ||
type ConnectBankAccountWithPlaidParams = { |
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.
Why is this type defined inline while other types exist in separate files or at the top of this file? My understanding was that all types should exist in their own file.
const commandName = 'AddPersonalBankAccount'; | ||
|
||
const parameters = { | ||
type AddPersonalBankAccountParams = { |
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.
Same
}, | ||
); | ||
function deletePaymentBankAccount(bankAccountID: number) { | ||
type DeletePaymentBankAccountParams = {bankAccountID: number}; |
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.
Same - OK, if I see any more, I'm going to skip commenting on them.
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.
Ah, I see now that maybe this is the pattern to follow for params to API calls?
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 wonder if there is a good reason for still not wanting to define them in their own files... 🤔
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.
@tgolen Yeah, we agreed to define API params types like this for now. As I know, we are going to improve API typing in the future, so it can be put in a separate file then
^^ @blazejkustra @kubabutkiewicz
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.
Yes @tgolen there was a message from @fabioh8010 that we will follow this for now
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'm totally on board with that approach. Thanks for making me aware of it!
src/libs/actions/BankAccounts.ts
Outdated
} | ||
|
||
function openReimbursementAccountPage(stepToOpen, subStep, localCurrentStep) { | ||
const onyxData = { | ||
function openReimbursementAccountPage(stepToOpen: string, subStep: string, localCurrentStep: string) { |
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 looks like stepToOpen
comes from this:
App/src/pages/ReimbursementAccount/ReimbursementAccountPage.js
Lines 209 to 224 in 34c2a27
switch (lodashGet(this.props.route, ['params', 'stepToOpen'], '')) { | |
case 'new': | |
return CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT; | |
case 'company': | |
return CONST.BANK_ACCOUNT.STEP.COMPANY; | |
case 'personal-information': | |
return CONST.BANK_ACCOUNT.STEP.REQUESTOR; | |
case 'contract': | |
return CONST.BANK_ACCOUNT.STEP.ACH_CONTRACT; | |
case 'validate': | |
return CONST.BANK_ACCOUNT.STEP.VALIDATION; | |
case 'enable': | |
return CONST.BANK_ACCOUNT.STEP.ENABLE; | |
default: | |
return ''; | |
} |
Instead of using a type of string
, how about using something more like ValueOf<typeof CONST.BANK_ACCOUNT.STEP>
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.
@tgolen Updated
src/libs/actions/BankAccounts.ts
Outdated
@@ -300,122 +305,104 @@ function openReimbursementAccountPage(stepToOpen, subStep, localCurrentStep) { | |||
], | |||
}; | |||
|
|||
const param = { | |||
type OpenReimbursementAccountPageParams = { | |||
stepToOpen: string; |
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.
Same comment here about using ValueOf
function updateBeneficialOwnersForBankAccount(params) { | ||
API.write('UpdateBeneficialOwnersForBankAccount', {...params}, getVBBADataForOnyx()); | ||
function updateBeneficialOwnersForBankAccount(params: ACHContractStepProps) { | ||
API.write('UpdateBeneficialOwnersForBankAccount', params, getVBBADataForOnyx()); |
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.
Are you sure there is no problem with removing the spread operator (like, maybe it was there to ensure something wasn't passed by reference)?
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.
@tgolen I've found only one usage of this function and it looks like it should be ok
https://github.com/Expensify/App/blob/main/src/pages/ReimbursementAccount/ACHContractStep.js#L137
Ah, sorry. There is a conflict now. |
# Conflicts: # src/libs/actions/BankAccounts.ts
# Conflicts: # src/libs/actions/BankAccounts.ts
@tgolen I've resolved conflicts and fixed a couple of TS errors after that |
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.
OK, thanks! Could you also please remove No QA
from the issue title? QA should be able to test out this flow to ensure there were no regressions.
@tgolen Sure, done |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/tgolen in version: 1.3.85-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.85-4 🚀
|
🚀 Deployed to staging by https://github.com/tgolen in version: 1.3.86-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.86-5 🚀
|
Details
Fixed Issues
$ #24903
PROPOSAL: N/A
Tests
Make sure you are using the staging server (Settings -> Preferences -> Use staging server)
Offline tests
N/A
QA Steps
Make sure you are using the staging server (Settings -> Preferences -> Use staging server)
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)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.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
Web
web.mov
Mobile Web - Chrome
Android.google.mov
Mobile Web - Safari
ios.safari.mov
Desktop
Desktop.mov
iOS
ios.mov
Android
android.mp4