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

Remove underscore from expensify-common and reduce usage of lodash and jQuery #718

Conversation

Skalakid
Copy link
Contributor

@Skalakid Skalakid commented Jun 12, 2024

Rebased version of #704 PR, that were made to sign all commits. Slack thread

The original goal of this PR was to remove underscore, jQuery, and lodash from files that are connected to ExpensiMark so we can workletize these functions and enable the react-native-live-markdown to run parser on UI thread. We wanted to block usage of these libraries in the whole repo, however without knowing the use cases and without being able to test it inside the private Expensify reposthat uses some of these files, we decided to:

  • remove whole underscore from repo, since it's quite easy to do without testing
  • replace more complex underscore fucntions with lodash versions because it doesn;t make. sene to re-write them
  • remove all these 3 libraries from: ExpensiMark, Logger, Src so we can workletize functions from these files and run them on UI thread in react-native-live-markdown

Fixed Issues

Expensify/App#42494

Tests

  1. What unit/integration tests cover your change? What autoQA tests cover your change?
  2. What tests did you perform that validates your changed worked?

QA

  1. What does QA need to do to validate your changes?
  2. What areas to they need to test for regressions?

@Skalakid Skalakid marked this pull request as ready for review June 12, 2024 17:38
@Skalakid Skalakid requested a review from a team as a code owner June 12, 2024 17:38
@melvin-bot melvin-bot bot requested review from youssef-lr and removed request for a team June 12, 2024 17:38
@Skalakid
Copy link
Contributor Author

@puneetlath here is a new PR that have all commit signed

@puneetlath
Copy link
Contributor

@hungvu193 can you give it a once-over for good measure?

@puneetlath puneetlath requested review from puneetlath and removed request for youssef-lr June 12, 2024 19:03
@Skalakid Skalakid changed the title Remove underscore and reduce usage of lodash and j query Remove underscore from expensify-common and reduce usage of lodash and jQuery Jun 12, 2024
@hungvu193
Copy link
Contributor

Sure

lib/ExpensiMark.js Show resolved Hide resolved
lib/Func.jsx Show resolved Hide resolved
lib/PubSub.jsx Outdated Show resolved Hide resolved
lib/components/form/element/combobox.js Outdated Show resolved Hide resolved
lib/components/form/element/combobox.js Outdated Show resolved Hide resolved
lib/components/form/element/combobox.js Outdated Show resolved Hide resolved
Copy link
Contributor

@hungvu193 hungvu193 left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@Skalakid
Copy link
Contributor Author

All yours @puneetlath :D

@puneetlath puneetlath requested a review from dangrous June 13, 2024 14:37
Copy link
Contributor

@dangrous dangrous left a comment

Choose a reason for hiding this comment

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

LGTM!

Re: removing those rules, we should maybe call that out publicly, so that people know we should still try to follow them even if the linter says no. What do you think, @puneetlath?

@blazejkustra
Copy link
Contributor

@puneetlath @dangrous Who is brave enough to merge this one? 😄

@puneetlath puneetlath merged commit 96708b9 into Expensify:main Jun 14, 2024
6 checks passed
@puneetlath
Copy link
Contributor

@dangrous That sounds good. @Skalakid could you make an announcement about that in #expensify-open-source?

Copy link

🚀Published to npm in v2.0.15

@Skalakid
Copy link
Contributor Author

@puneetlath @dangrous I've posted the announcement on #expensify-open-source

@hungvu193
Copy link
Contributor

Should we also upgrade expensify-common version in EApp?

@blazejkustra
Copy link
Contributor

I think we should update in both LM and E/App @hungvu193. It's already merged in LM, here's the PR.

@hungvu193
Copy link
Contributor

Thanks for your information 😄

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.

6 participants