-
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
Remove underscore usage #40346
Remove underscore usage #40346
Conversation
@hungvu193 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] |
A few notes:
a. BaseOptionsSelector is deprecated and I see that we are replacing it with SelectionList in this issue, but I don't see the plan to do that in BaseOptionsSelector usages left are in b. I don't see any issue migrating |
@bernhardoj Lint is failing 👀 |
Couple things that I noticed:
|
this.relatedTarget = null; | ||
this.accessibilityRoles = _.values(CONST.ROLE); | ||
this.accessibilityRoles = Object.values(CONST.ROLE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's disable the lint error with a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better if we removed the ESLint config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or should i just use lodash here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use lodash
Since we will have 2,3 reviewers here, @bernhardoj can you also include my suggestion here in this PR?
|
Done, but after removing the underscore from
@hungvu193 Removed from eslint, but I don't see any underscore-related config in webpack.
@mountiny Do we need to update this? |
.eslintrc.js
Outdated
@@ -238,7 +238,6 @@ module.exports = { | |||
'jsdoc/no-types': 'error', | |||
'rulesdir/no-default-props': 'error', | |||
'import/no-extraneous-dependencies': 'off', | |||
'rulesdir/prefer-underscore-method': 'off', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blazejkustra We got lint errors when I removed this. Do we want to update eslint-config-expensify
to remove this rule or keep turning this rule off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should eslint-config-expensify
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mountiny can you confirm?
I will update this tomorrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have turned this off and added back npm install underscore
in CIGitLogicTest
for now since we don't have any answer yet. I think you can start reviewing it. @hungvu193
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, I ask for the help in slack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I'll review in a while
We have conflicts here @bernhardoj |
Conflicts solved.
Deleted both files |
Ready for a review @hungvu193 |
Sure thing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I left few NAB comments/
!this.props.isReadOnly && (this.props.shouldShowConfirmButton || this.props.footerContent) && !(this.props.canSelectMultipleOptions && _.isEmpty(this.props.selectedOptions)); | ||
const defaultConfirmButtonText = _.isUndefined(this.props.confirmButtonText) ? this.props.translate('common.confirm') : this.props.confirmButtonText; | ||
!this.props.isReadOnly && (this.props.shouldShowConfirmButton || this.props.footerContent) && !(this.props.canSelectMultipleOptions && this.props.selectedOptions.length === 0); | ||
const defaultConfirmButtonText = typeof this.props.confirmButtonText === 'undefined' ? this.props.translate('common.confirm') : this.props.confirmButtonText; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB, I think this.props.confirmButtonText === 'undefined'
is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, updated!
src/pages/iou/request/MoneyTemporaryForRefactorRequestParticipantsSelector.js
Show resolved
Hide resolved
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid.movAndroid: mWeb ChromemChrome.moviOS: NativeScreen.Recording.2024-04-22.at.12.09.50.moviOS: mWeb SafarimSafari.movMacOS: Chrome / Safarichrome.movMacOS: Desktopdesktop.mov |
We had some conflicts again @bernhardoj |
@hungvu193 conflicts solved |
tests/unit/CIGitLogicTest.sh
Outdated
@@ -77,7 +76,7 @@ function bump_version { | |||
setup_git_as_osbotify | |||
git switch main | |||
npm --no-git-tag-version version "$(ts-node "$bumpVersion" "$(print_version)" "$1")" | |||
git add package.json package-lock.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the package-lock.json doesn't exist anymore after removing the npm install underscore
, so not adding it to the git fix the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't remove these two lines, maybe a better idea would be to install a different package?
npm install lodash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, totally agree with that. Updated!
tests/unit/CIGitLogicTest.sh
Outdated
@@ -77,7 +76,7 @@ function bump_version { | |||
setup_git_as_osbotify | |||
git switch main | |||
npm --no-git-tag-version version "$(ts-node "$bumpVersion" "$(print_version)" "$1")" | |||
git add package.json package-lock.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't remove these two lines, maybe a better idea would be to install a different package?
npm install lodash
## Accessing Object Properties and Default Values | ||
|
||
Use `lodashGet()` to safely access object properties and `||` to short circuit null or undefined values that are not guaranteed to exist in a consistent way throughout the codebase. In the rare case that you want to consider a falsy value as usable and the `||` operator prevents this then be explicit about this in your code and check for the type using an underscore method e.g. `_.isBoolean(value)` or `_.isEqual(0)`. | ||
Use `lodashGet()` to safely access object properties and `||` to short circuit null or undefined values that are not guaranteed to exist in a consistent way throughout the codebase. In the rare case that you want to consider a falsy value as usable and the `||` operator prevents this then be explicit about this in your code and check for the type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are going to adjust this markdown in a separate PR, so we can leave it as it is now 👍
contributingGuides/STYLE.md
Outdated
@@ -448,7 +419,7 @@ const propTypes = { | |||
|
|||
### Important Note: | |||
|
|||
In React Native, one **must not** attempt to falsey-check a string for an inline ternary. Even if it's in curly braces, React Native will try to render it as a `<Text>` node and most likely throw an error about trying to render text outside of a `<Text>` component. Use `_.isEmpty()` instead. | |||
In React Native, one **must not** attempt to falsey-check a string for an inline ternary. Even if it's in curly braces, React Native will try to render it as a `<Text>` node and most likely throw an error about trying to render text outside of a `<Text>` component. Use `.length > 0` instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not double negation?
In React Native, one **must not** attempt to falsey-check a string for an inline ternary. Even if it's in curly braces, React Native will try to render it as a `<Text>` node and most likely throw an error about trying to render text outside of a `<Text>` component. Use `.length > 0` instead. | |
In React Native, one **must not** attempt to falsey-check a string for an inline ternary. Even if it's in curly braces, React Native will try to render it as a `<Text>` node and most likely throw an error about trying to render text outside of a `<Text>` component. Use `!!` instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, didn't think of that. I was just converting isEmpty to .length > 0.
Updated
src/pages/iou/request/MoneyTemporaryForRefactorRequestParticipantsSelector.js
Show resolved
Hide resolved
e626327
to
c6ee94f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All yours @mountiny
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, great job on this one
@@ -112,38 +112,9 @@ if (someCondition) { | |||
} | |||
``` | |||
|
|||
## Object / Array Methods | |||
|
|||
We have standardized on using [underscore.js](https://underscorejs.org/) methods for objects and collections instead of the native [Array instance methods](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array#instance_methods). This is mostly to maintain consistency, but there are some type safety features and conveniences that underscore methods provide us e.g. the ability to iterate over an object and the lack of a `TypeError` thrown if a variable is `undefined`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we still add this information in here? Seems like there is a preference and posibility of using lodash so it makes sense to leave a guide here in terms of what is preferred
## Accessing Object Properties and Default Values | ||
|
||
Use `lodashGet()` to safely access object properties and `||` to short circuit null or undefined values that are not guaranteed to exist in a consistent way throughout the codebase. In the rare case that you want to consider a falsy value as usable and the `||` operator prevents this then be explicit about this in your code and check for the type using an underscore method e.g. `_.isBoolean(value)` or `_.isEqual(0)`. | ||
Use `lodashGet()` to safely access object properties and `||` to short circuit null or undefined values that are not guaranteed to exist in a consistent way throughout the codebase. In the rare case that you want to consider a falsy value as usable and the `||` operator prevents this then be explicit about this in your code and check for the type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 1.4.65-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.65-5 🚀
|
Details
Removing underscore usage from the codebase.
Fixed Issues
$ #39121
PROPOSAL: #39121 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2024-04-17.at.19.59.11.mov
MacOS: Desktop