Remove underscore from expensify-common and reduce usage of lodash and jQuery #704
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
Skalakid
force-pushed
the
remove-jQuery-underscore-lodash
branch
from
6d15818
to
4407b0b
Compare
|
I can review this one as C+ @puneetlath |
|
@puneetlath I'm happy to do so - either instead or in addition - but let me know if you want to be the official reviewer here since it looks like you have the context! |
|
Ah yeah, here's the slack convo: |
|
@dangrous yeah I can take this one since I'm assigned to the issue. Though you're welcome to review as well if you like! |
|
Linting is failing @Skalakid |
|
@hungvu193 You can continue reviewing it's connected to this (@Skalakid needs to restore the rule and it'll be fine) |
There was a problem hiding this comment.
Looks good to me, @hungvu193 please double check that every lodash/underscore method is replaced correctly + watch out for empty values like null/undefined as these could break the logic.
@Skalakid Should we uninstall underscore nad lodash perhaps?
|
Looks good to me as well! |
|
@hungvu193 @blazejkustra All lint errors should be fixed now and I uninstalled |
|
@puneetlath @dangrous What are the next steps here? |
|
|
||
| // Use this deferred lib so we don't have a dependency on jQuery (so we can use this module in mobile) | ||
| import {Deferred} from 'simply-deferred'; | ||
| import {has} from 'lodash'; |
There was a problem hiding this comment.
maybe I'm misinterpreting the issue goal but shouldn't we be removing anything from lodash?
There was a problem hiding this comment.
Oh sorry, I forgot to update the title of the PR and its description. We decided to remove all these libraries only inside files that we will be using in react-native-live-markdown and execute on the UI thread (ExpensiMark, Src etc). Also, we decided to move more complex underscore functions to lodash, like during migration to TS in E/App, because it doesn't make sense to rewrite them on our own in the utils file
There was a problem hiding this comment.
I updated the PR description
| 'arrow-body-style': 'off', | ||
| 'es/no-nullish-coalescing-operators': 'off', | ||
| 'rulesdir/prefer-underscore-method': 'off', | ||
| 'es/no-optional-chaining': 'off', |
There was a problem hiding this comment.
NAB - why are we turning off these rules?
There was a problem hiding this comment.
We turned off some ESlint rules because in some cases we had to use some JS features that were blocked by them, to make functions work the same
| @@ -1,6 +1,5 @@ | |||
| import $ from 'jquery'; | |||
There was a problem hiding this comment.
should we be removing/replacing this too?
There was a problem hiding this comment.
We decided to not migrate it because we don't have access to repos that are actually using this Network file, and we couldn't test it. Replacing jQuery can introduce many critical regressions and require bigger logic changes
| import has from 'lodash/has'; | ||
| import {once} from 'lodash'; |
There was a problem hiding this comment.
same here - aren't we removing lodash?
There was a problem hiding this comment.
The same answer from above applies to it
Skalakid
changed the title
|
I am closing this PR since I created a rebased version of it, that has all commits signed. Please continue reviewing/discussions in the new PR |
The original goal of this PR was to remove
underscore,jQuery, andlodashfrom files that are connected toExpensiMarkso we can workletize these functions and enable thereact-native-live-markdownto 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:underscorefrom repo, since it's quite easy to do without testingunderscorefucntions withlodashversions because it doesn;t make. sene to re-write themExpensiMark,Logger,Srcso we can workletize functions from these files and run them on UI thread inreact-native-live-markdownFixed Issues
Expensify/App#42494
Tests
QA