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.

Sign up for GitHub

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

Jump to bottom

lib: remove unnecessary optional chaining #55728

Merged
merged 3 commits into from
Nov 7, 2024
Merged

lib: remove unnecessary optional chaining #55728

merged 3 commits into from
Nov 7, 2024

Conversation

gurgunday
Copy link
Contributor

From the V8 blog:

Still, be considerate about using more than one optional chaining operator in a single chain. If a value is guaranteed to not be nullish, then using ?. to access properties on it is discouraged.

It adds more bytecode to each property access, in all these cases we know that the variables are not nullish

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 5, 2024
@codecov Codecov
Copy link

codecov bot commented Nov 5, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (58a7b00) to head (557ac67).
Report is 22 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #55728   +/-   ##
=======================================
  Coverage   88.40%   88.40%           
=======================================
  Files         654      654           
  Lines      187655   187747   +92     
  Branches    36111    36123   +12     
=======================================
+ Hits       165889   165984   +95     
- Misses      14996    15007   +11     
+ Partials     6770     6756   -14
Files with missing lines Coverage Δ
lib/events.js 99.75% <100.00%> (ø)
lib/fs.js 98.14% <100.00%> (ø)
lib/internal/fs/promises.js 98.24% <100.00%> (ø)
lib/internal/fs/read/context.js 94.57% <100.00%> (ø)
lib/internal/fs/watchers.js 85.78% <100.00%> (ø)
lib/timers/promises.js 99.15% <100.00%> (ø)

... and 34 files with indirect coverage changes

@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 5, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 5, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63435/

aduh95
aduh95 approved these changes Nov 5, 2024
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

I wonder if we could have a lint rule for that, that'd be neat

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63441/

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 6, 2024
LiviaMedeiros
LiviaMedeiros approved these changes Nov 6, 2024
View reviewed changes
Copy link
Contributor

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

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

LGTM as is, but pointing out that there are still more unnecessary ?.s around.
Also perhaps there are implicitly redundant optional chainings in internal functions, but we have to ensure that every { signal: null } case is covered by tests before removing them.

lib/events.js Outdated Show resolved Hide resolved
lib/events.js Show resolved Hide resolved
@gurgunday @LiviaMedeiros
Update lib/events.js
62f7617 
Co-authored-by: Livia Medeiros <[email protected]>
@gurgunday
Copy link
Contributor Author

@LiviaMedeiros @aduh95 I applied the first suggestion :)

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@LiviaMedeiros LiviaMedeiros added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Nov 6, 2024
aduh95
aduh95 reviewed Nov 6, 2024
View reviewed changes
lib/events.js Outdated Show resolved Hide resolved
@gurgunday @aduh95
Update lib/events.js
557ac67 
Co-authored-by: Antoine du Hamel <[email protected]>
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@gurgunday
Copy link
Contributor Author

@aduh95 can you relaunch the failed ones?

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63461/

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 7, 2024
@nodejs-github-bot nodejs-github-bot merged commit 7788999 into nodejs:main Nov 7, 2024
51 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 7788999

@gurgunday gurgunday deleted the abort branch November 9, 2024 11:30
aduh95 pushed a commit that referenced this pull request Nov 16, 2024
@gurgunday @aduh95
lib: remove unnecessary optional chaining
cfe28b1 
PR-URL: #55728
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 19, 2024
Merged
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
@gurgunday @tpoisseau
lib: remove unnecessary optional chaining
617601f 
PR-URL: nodejs#55728
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Nov 26, 2024
@gurgunday @Ceres6
lib: remove unnecessary optional chaining
22cfcb3 
PR-URL: nodejs#55728
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
ruyadorno pushed a commit that referenced this pull request Nov 27, 2024
@gurgunday @ruyadorno
lib: remove unnecessary optional chaining
ca73a1d 
PR-URL: #55728
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
ruyadorno pushed a commit that referenced this pull request Nov 27, 2024
@gurgunday @ruyadorno
lib: remove unnecessary optional chaining
995db8f 
PR-URL: #55728
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
ruyadorno pushed a commit that referenced this pull request Nov 27, 2024
@gurgunday @ruyadorno
lib: remove unnecessary optional chaining
cefce4c 
PR-URL: #55728
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: LiviaMedeiros <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@nodejs-github-bot nodejs-github-bot mentioned this pull request Nov 27, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@LiviaMedeiros LiviaMedeiros LiviaMedeiros approved these changes

@Flarna Flarna Flarna approved these changes

@trivikr trivikr trivikr approved these changes

@anonrig anonrig anonrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@gurgunday @nodejs-github-bot @jasnell @lpinca @anonrig @aduh95 @trivikr @Flarna @LiviaMedeiros