-
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
[HOLD] Revert "[TS migration] Migrate 'PersonalDetailsUtils.js' lib to TypeScript" #33438
Conversation
@jasperhuangg 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] |
@iwiznia could you please share a piece of context here as well?🙏🏼 |
Performance in heavy accounts WITH lots of personal details (aka internal employee accounts) has severely degraded since this migration hit staging 3 days ago. Performance in heavy accounts WITHOUT lots of personal details appears to be unaffected by the performance regression. |
@iwiznia there appear to be conflicts now, can you sort those out? |
Heading OOO for the holidays, so it might be best to assign another engineer |
Conflict resolved. Let's first confirm with the build if this revert actually fixes the issue |
@TMisiukiewicz please could you work with @MonilBhavsar on this today? |
@BartoszGrajdek Tagging you as you was the author of the PR. |
Not sure why it's not building. I'll run this against prod DB on my account and see if it fixes the issue |
Running the build |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
@ 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] |
@hayata-suenaga 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] |
Hi, author of the PR here 😅 As you can guess I'm not really a fan of having it reverted, so I'd love to pinpoint what exactly is causing the performance issue. My guess is that it's most likely due to some of the underscore/lodash removals, if you could provide some reproduction steps I would love to help out 🙏🏻 |
I don't know the steps exactly. I think you have to have lots of people you know, so the list is large. @muttmuure can you provide @BartoszGrajdek with steps to reproduce the issue please? |
cc: @mountiny as you're author of this revert |
The repro steps so far are:
|
So I guess that high-traffic account won't be enough for this, right? Since you can see only email address of the contact |
@iwiznia i think we might be able to close this one in case this was resolved by this revert pr #33496 which was hard to confirm because people are out but from what Callstack developer suggested the new phone number methods/regexp just make the search page explode if you have thousands of personal details with phone number as primary login |
Oh wow! That's nice to hear, closing then. |
Reverts #30169
Context https://expensify.slack.com/archives/C03SSAQ7P/p1703183739761709
cc @thesahindia @blazejkustra @MonilBhavsar @fabioh8010 @paultsimura