-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 'HttpUtils.js' lib to TypeScript #28691
[TS migration] Migrate 'HttpUtils.js' lib to TypeScript #28691
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.
LGTM
# Conflicts: # src/libs/HttpUtils.ts # src/types/onyx/Response.ts
Co-authored-by: Jakub Butkiewicz <jakub.butkiewicz94@gmail.com>
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 comment, besides LGTM.
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid-native-2023-11-21_14.24.30.mp4Android: mWeb Chromeandroid-chrome-2023-11-21_14.22.11.mp4iOS: Nativeios-native-2023-11-21_14.16.03.mp4iOS: mWeb Safariios-safari-2023-11-21_14.14.05.mp4MacOS: Chrome / Safaridesktop-chrome-2023-11-21_13.53.42.mp4MacOS: Desktopdesktop-app-2023-11-21_14.11.27.mp4 |
@fvlvte Any particular tests for this? |
@fvlvte Friendly bump to add some test steps to this, so I can check off the box 😉 |
@jjcoffee after investigating which parts of code it's touching, i think running of automated tests is enough - since it's using it - and the Request class aswell so in general if there's no issues in navigating trough app screens it's working good (calling API properly), i've edge cased 429 with logging in too many times, it works also good. |
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 #24890 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
*/ | ||
function processHTTPRequest(url, method = 'get', body = null, canCancel = true) { | ||
function processHTTPRequest(url: string, method: RequestType = 'get', body: FormData | null = null, canCancel = true): Promise<Response> { |
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.
Shouldn't we be adding the type of canCancel
as boolean?
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.
We don't need to explicit type this parameter because we use default value of boolean type and it will inherit the type from it.
src/libs/HttpUtils.ts
Outdated
CONST.HTTP_STATUS.INTERNAL_SERVER_ERROR, | ||
CONST.HTTP_STATUS.BAD_GATEWAY, | ||
CONST.HTTP_STATUS.GATEWAY_TIMEOUT, | ||
CONST.HTTP_STATUS.UNKNOWN_ERROR, | ||
]; | ||
if (_.contains(serviceInterruptedStatuses, response.status)) { | ||
if (serviceInterruptedStatuses.some((status) => status === response.status)) { |
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.
NAB but kind of clearer, no?
if (serviceInterruptedStatuses.some((status) => status === response.status)) { | |
if (serviceInterruptedStatuses.indexOf(response.status) > -1) { |
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.
Agreed, applied.
*/ | ||
function xhr(command, data, type = CONST.NETWORK.METHOD.POST, shouldUseSecure = false) { | ||
function xhr(command: string, data: Record<string, unknown>, type: RequestType = CONST.NETWORK.METHOD.POST, shouldUseSecure = false): Promise<Response> { |
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 the boolean for shouldUseSecure
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 as above.
src/libs/HttpUtils.ts
Outdated
// Do not send undefined request parameters to our API. They will be processed as strings of 'undefined'. | ||
if (_.isUndefined(val)) { | ||
Object.keys(data).forEach((key) => { | ||
if (!data[key]) { |
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 is not equivalent to the old code... this will remove any 0
, false
, null
or ""
while the old code would not
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.
That's true, changed to typeof.
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.
@jjcoffee FYI since you missed this in your review and I have the feeling this would've broken a ton of things.
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.
Damn, good catch! Sorry for missing that 🙇
✋ 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/iwiznia in version: 1.4.2-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.2-3 🚀
|
Details
Migrated 'HttpUtils.js' lib to TypeScript.
Fixed Issues
$ #24890
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
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
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
Android: Native
android-native.mov
Android: mWeb Chrome
android-chrome.mov
iOS: Native
ios-native.mov
iOS: mWeb Safari
ios-safari.mov
MacOS: Chrome / Safari
chrome.mov
MacOS: Desktop
macos.mov