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

[$250] Implement Three Suggested Changes from Callstack #9427

Closed
Luke9389 opened this issue Jun 13, 2022 · 25 comments
Closed

[$250] Implement Three Suggested Changes from Callstack #9427

Luke9389 opened this issue Jun 13, 2022 · 25 comments
Assignees
Labels
Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@Luke9389
Copy link
Contributor

Luke9389 commented Jun 13, 2022

Background

Callstack is vendor that we've engaged to help with app performance. They've needed to delay our engagement until September, but in the mean time gave a few tips for us that they think would be helpful.

Suggested Changes

  • The @react-native-community/cli dep shouldn't be added directly. It's a transitive dependency of react-native.
    Note: No longer adding @react-native-community/cli directly in package.json

  • Both underscore and lodash are used. These are big libraries with the same purpose so you can pick one instead (we suggest lodash in this case)

  • Remove Intl polyfills (@formatjs/intl*) usage, which can add up to 6MB of unicode data into the JS bundle. The Hermes version you're using supports Intl object by default, so polyfill is an unnecessary overhead

Game Plan

I'll bring these up for discussions with engineering to verify that we should proceed, and then implement the changes that we greenlight.

@Luke9389 Luke9389 added the Weekly KSv2 label Jun 13, 2022
@Luke9389 Luke9389 self-assigned this Jun 13, 2022
@Luke9389
Copy link
Contributor Author

Brought this up in eng-chat here

@Luke9389
Copy link
Contributor Author

After discussing this in the thread linked above, we've decided to strike number 2 from the list.

@Luke9389
Copy link
Contributor Author

Coming back to this issue this week.

@melvin-bot melvin-bot bot removed the Overdue label Jun 27, 2022
@melvin-bot melvin-bot bot added the Overdue label Jul 5, 2022
@Luke9389
Copy link
Contributor Author

Luke9389 commented Jul 7, 2022

back soon

@melvin-bot melvin-bot bot removed the Overdue label Jul 7, 2022
@parasharrajat
Copy link
Member

parasharrajat commented Jul 8, 2022

One more thing that I can think of is to prevent the use of inline prop values(non-static) as much as possible.

  1. No Inline callbacks.
  2. No inline objects.

Object literals will recreate a new object on each render and due to a change of prop value, the underlying component will rerender.

@melvin-bot melvin-bot bot added the Overdue label Jul 18, 2022
@Luke9389
Copy link
Contributor Author

Thanks @parasharrajat, I'll check out the impact of those suggestions 👍

@melvin-bot melvin-bot bot removed the Overdue label Jul 20, 2022
@melvin-bot melvin-bot bot added the Overdue label Jul 28, 2022
@Luke9389
Copy link
Contributor Author

Gonna loop back around to this

@melvin-bot melvin-bot bot removed the Overdue label Jul 28, 2022
@melvin-bot melvin-bot bot added the Overdue label Aug 8, 2022
@Luke9389
Copy link
Contributor Author

Luke9389 commented Aug 8, 2022

Hoping to get to this soon.

@melvin-bot melvin-bot bot removed the Overdue label Aug 8, 2022
@melvin-bot melvin-bot bot added the Overdue label Aug 19, 2022
@Luke9389
Copy link
Contributor Author

Hoping to get to this soon.

@melvin-bot melvin-bot bot removed the Overdue label Aug 23, 2022
@melvin-bot melvin-bot bot added the Overdue label Sep 1, 2022
@Luke9389 Luke9389 added Internal Requires API changes or must be handled by Expensify staff ContributorPlusReview labels Sep 1, 2022
@melvin-bot melvin-bot bot removed the Overdue label Sep 1, 2022
@Luke9389
Copy link
Contributor Author

Luke9389 commented Sep 1, 2022

C+ would you be willing to help test this PR for me? #10750

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Sep 2, 2022
@melvin-bot
Copy link

melvin-bot bot commented Sep 2, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 2, 2022

Current assignee @Luke9389 is eligible for the Exported assigner, not assigning anyone new.

@melvin-bot melvin-bot bot changed the title Implement Three Suggested Changes from Callstack [$250] Implement Three Suggested Changes from Callstack Sep 2, 2022
@jboniface
Copy link

@mananjadhav job post here: https://www.upwork.com/jobs/~0175fa858fa8779088

@mananjadhav
Copy link
Collaborator

Applied @jboniface

@melvin-bot melvin-bot bot added the Overdue label Sep 12, 2022
@jboniface
Copy link

any update here? @Luke9389 / @mananjadhav

@melvin-bot melvin-bot bot removed the Overdue label Sep 12, 2022
@mananjadhav
Copy link
Collaborator

I've reviewed and shared a bug with the PR. Waiting for the changes from @Luke9389. But mostly we need a RN upgrade from 0.69.0 to 0.70.0.

@Luke9389
Copy link
Contributor Author

Hey @jboniface & @mananjadhav, I still haven't had a cycle to come back to this. I'll be updating soon.

@jboniface jboniface removed their assignment Sep 15, 2022
@melvin-bot melvin-bot bot added the Overdue label Sep 26, 2022
@Luke9389
Copy link
Contributor Author

I'm going to close this in favor of all the performance updates we're doing with margelo. I still haven't gotten the app to run without the format/intl dependency, and that's the only outstanding task from callstack of the original three.
@parasharrajat I think it's worth bringing this suggestion up in the open source room. That way we can make it a separate ticket if we decide to do it.

@melvin-bot melvin-bot bot removed the Overdue label Sep 26, 2022
@Luke9389
Copy link
Contributor Author

Thanks for helping test here @mananjadhav,
@Expensify/contributor-management would any of you be willing to pay out this issue for @mananjadhav?

@JmillsExpensify
Copy link

I'll do it. Assigning myself now.

@JmillsExpensify JmillsExpensify self-assigned this Sep 26, 2022
@Luke9389
Copy link
Contributor Author

Thanks!

@JmillsExpensify
Copy link

@mananjadhav Send you a contract offer here: https://www.upwork.com/jobs/~01967be1fa14712bf3

@JmillsExpensify
Copy link

Paid via Upwork

@JmillsExpensify
Copy link

Whoops. Closing since this is now paid!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
None yet
Development

No branches or pull requests

5 participants