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.

Sign up for GitHub

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

Jump to bottom

[$500] [HOLD for payment 2023-11-21] Remove multiple withOnyx nestings #28824

Closed
1 task done
tgolen opened this issue Oct 4, 2023 · 37 comments
Closed
1 task done

[$500] [HOLD for payment 2023-11-21] Remove multiple withOnyx nestings #28824

tgolen opened this issue Oct 4, 2023 · 37 comments
Assignees
@tgolen @laurenreidexpensify @situchan
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Internal Requires API changes or must be handled by Expensify staff NewFeature Something to build that is a new item.

Comments

@tgolen
Copy link
Contributor

tgolen commented Oct 4, 2023 •
edited by melvin-bot bot
Loading

Problem

Our code is using multiple nested withOnyx HOCs in order to load onyx keys that are dependent on other onyx keys. I previously fixed it with this PR but that lead to massive performance problems that essentially bricked the application (locked everything up) so it had to be reverted.

Why this matters

This was supposed to be a performance improvement and a code-cleanup.

Solution

Figure out what the performance problem with the fix is and see about solving it.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0135e530369ae2886e
  • Upwork Job ID: 1709587003530883072
  • Last Price Increase: 2023-11-28
  • Automatic offers:
    • situchan | Contributor | 27862747
@tgolen tgolen added Engineering Daily KSv2 Internal Requires API changes or must be handled by Expensify staff NewFeature Something to build that is a new item. labels Oct 4, 2023
@tgolen tgolen self-assigned this Oct 4, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

Job added to Upwork: https://www.upwork.com/jobs/~0135e530369ae2886e

@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

Triggered auto assignment to @joekaufmanexpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Oct 4, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

Triggered auto assignment to Contributor Plus for review of internal employee PR - @jjcoffee (Internal)

@tgolen
Copy link
Contributor Author

tgolen commented Oct 4, 2023

I'm doing this internally, so no need for bugzero or C+ yet. I'll unassign you both for now.

@tgolen tgolen added Daily KSv2 and removed Weekly KSv2 labels Oct 4, 2023
This was referenced Oct 4, 2023
Merged
Merged
@tgolen
Copy link
Contributor Author

tgolen commented Oct 5, 2023

I opened the PR for this yesterday and I will continue testing today with a large account to ensure there are no performance problems.

@tgolen tgolen added the Reviewing Has a PR in review label Oct 6, 2023
@tgolen
Copy link
Contributor Author

tgolen commented Oct 6, 2023

My testing has gone well so I don't forsee there being any performance impact with this change. I'll keep bumping the PR for review.

@tgolen
Copy link
Contributor Author

tgolen commented Oct 11, 2023

The Onyx change is currently being rolled out in a separate PR. Once it's rolled out, I will get #28866 ready for review.

@tgolen
Copy link
Contributor Author

tgolen commented Oct 17, 2023

It looks like #28894 has been deployed to staging, so I can get back on this again.

@tgolen tgolen mentioned this issue Oct 17, 2023
Merged
59 tasks
@tgolen
Copy link
Contributor Author

tgolen commented Oct 17, 2023

That was a mistake. #29169 is the PR with the version bump that this is waiting on.

@tgolen tgolen removed the Reviewing Has a PR in review label Oct 19, 2023
@tgolen
Copy link
Contributor Author

tgolen commented Oct 19, 2023

I'm going to drop this to weekly since it's on HOLD and it's not super critical.

@melvin-bot melvin-bot bot added the Overdue label Oct 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Oct 23, 2023

@tgolen Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Nov 14, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.98-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-11-21. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Nov 14, 2023

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@tgolen] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 21, 2023
@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Nov 24, 2023

@tgolen Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Nov 24, 2023
@tgolen tgolen added the Bug Something is broken. Auto assigns a BugZero manager. label Nov 27, 2023
@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Nov 27, 2023

Triggered auto assignment to @laurenreidexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot removed the Overdue label Nov 27, 2023
@melvin-bot Melvin Bot

This comment was marked as off-topic.

Sign in to view
@tgolen
Copy link
Contributor Author

tgolen commented Nov 27, 2023

@laurenreidexpensify Can you please help with the payment for this one? @situchan needs paid for a C+ review of the PR.

@laurenreidexpensify laurenreidexpensify added the External Added to denote the issue can be worked on by a contributor label Nov 28, 2023
@melvin-bot melvin-bot bot changed the title [HOLD for payment 2023-11-21] Remove multiple withOnyx nestings [$500] [HOLD for payment 2023-11-21] Remove multiple withOnyx nestings Nov 28, 2023
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 28, 2023
@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Nov 28, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr (External)

@laurenreidexpensify laurenreidexpensify added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Nov 28, 2023
@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Nov 28, 2023

Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts (External)

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 28, 2023
@melvin-bot Melvin Bot
Copy link

melvin-bot bot commented Nov 28, 2023

📣 @situchan 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@laurenreidexpensify laurenreidexpensify mentioned this issue Nov 28, 2023
Closed
@laurenreidexpensify
Copy link
Contributor

Payment Summary:

C+ Review - @situchan $500 offer sent in Upwork - https://www.upwork.com/en-gb/jobs/~01376316f87bc09708

@laurenreidexpensify
Copy link
Contributor

Payment Summary:

C+ Review - @situchan $500 paid in Upwork

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tgolen tgolen

@laurenreidexpensify laurenreidexpensify

@situchan situchan

Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Internal Requires API changes or must be handled by Expensify staff NewFeature Something to build that is a new item.
Projects
[#whatsnext] #newdot-quality
Status: Done
Milestone
No milestone
Development

No branches or pull requests
8 participants
@tgolen @mollfpr @jjcoffee @allroundexperts @roryabraham @joekaufmanexpensify @laurenreidexpensify @situchan