Skip to content
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

Optimize loops in formatting personal details #7649

Merged
merged 1 commit into from
Feb 9, 2022

Conversation

tgolen
Copy link
Contributor

@tgolen tgolen commented Feb 9, 2022

Details

Fixed Issues

$ #7630

Tests / QA

Note: this can't really be QAed on iOS or Android since there will be no way to inspect the console

  1. Load the app
  2. Look in the JS console for Timing:expensify.cash.personal_details_formatted (need to have the verbose visibility setting turned on)
  3. Verify the number is low (ideally under 1000ms)
  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

Mobile Web

Same as web

Desktop

image

iOS

image

Android

image

@tgolen tgolen self-assigned this Feb 9, 2022
@tgolen tgolen marked this pull request as ready for review February 9, 2022 16:00
@tgolen tgolen requested a review from a team as a code owner February 9, 2022 16:00
@tgolen tgolen requested a review from luacmartins February 9, 2022 16:00
@MelvinBot MelvinBot requested review from nickmurray47 and removed request for a team February 9, 2022 16:00
const phoneNumber = lodashGet(personalDetailsResponse, 'phoneNumber', '');

return {
...finalObject,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything seems the same but this spread here
I reckon it was the bottleneck, if that so you can just swap it like this:

finalObject[login] = { 
  login,
  avatar,
  displayName,
};

return finalObject;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it was definitely the spread. I found it from looking at this and noticing the _objectSpread being called over and over.

image

I figured the rest of the changes, while not strictly necessary, would only help.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2022

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@luacmartins luacmartins merged commit 1923c3d into main Feb 9, 2022
@luacmartins luacmartins deleted the tgolen-fire-infiniteloops branch February 9, 2022 16:28
OSBotify pushed a commit that referenced this pull request Feb 9, 2022
Optimize loops in formatting personal details

(cherry picked from commit 1923c3d)
@OSBotify
Copy link
Contributor

OSBotify commented Feb 9, 2022

🚀 Deployed to production by @roryabraham in version: 1.1.37-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Feb 9, 2022

🚀 Cherry-picked to staging by @luacmartins in version: 1.1.37-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants