GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — cluster: replace forEach with for..of in lib/internal/cluster/ directory by jerome-benoit · Pull Request #50317 · nodejs/node

Conversation

@jerome-benoit

@jerome-benoit jerome-benoit commented Oct 21, 2023

Copy link
Copy Markdown
Contributor

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added cluster Issues and PRs related to the cluster subsystem. needs-ci PRs that need a full CI run. labels Oct 21, 2023
@TKDev7

TKDev7 commented Oct 22, 2023

Copy link
Copy Markdown

Out of curiosity why this change?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jerome-benoit

jerome-benoit commented Oct 22, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Out of curiosity why this change?

Coding style consistency.
There might also have performance implications at using implicitly iterator on collections (no boundary check, less memory footprint, ...), especially on large collection. forEach is probably using already iterator under the hood, with for...of it's explicit and does not depend on an helper implementation.
treblasft reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Oct 28, 2023
View reviewed changes
jerome-benoit added 4 commits May 11, 2024 21:20
@jerome-benoit @aduh95
cluster: replace forEach with for..of in lib/internal/cluster/ directory
001629e 
Signed-off-by: Jérôme Benoit <[email protected]>
@jerome-benoit @aduh95
refactor: silence linter
3deccf4 
Signed-off-by: Jérôme Benoit <[email protected]>
@jerome-benoit @aduh95
build: silence linter
0b1f164 
Signed-off-by: Jérôme Benoit <[email protected]>
@jerome-benoit @aduh95
fix: handles in a SafeMap
92a9ea3 
Signed-off-by: Jérôme Benoit <[email protected]>
@aduh95
aduh95 force-pushed the foreach2for-cluster branch from c348acb to 92a9ea3 Compare May 11, 2024 18:20
aduh95
aduh95 requested changes May 11, 2024
View reviewed changes
Comment thread lib/internal/cluster/primary.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@jerome-benoit @aduh95
Update lib/internal/cluster/primary.js
6950c59 
Co-authored-by: Antoine du Hamel <[email protected]>
@jerome-benoit
jerome-benoit requested a review from aduh95 May 11, 2024 19:26
aduh95
aduh95 approved these changes May 11, 2024
View reviewed changes
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels May 11, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label May 11, 2024
@nodejs-github-bot

nodejs-github-bot commented May 11, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59131/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented May 12, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59150/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels May 12, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels May 12, 2024
@nodejs-github-bot

nodejs-github-bot commented May 12, 2024

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/50317
✔  Done loading data for nodejs/node/pull/50317
----------------------------------- PR info ------------------------------------
Title      cluster: replace forEach with for..of in lib/internal/cluster/ directory (#50317)
Author     Jérôme Benoit  (@jerome-benoit, first-time contributor)
Branch     jerome-benoit:foreach2for-cluster -> nodejs:main
Labels     cluster, author ready, needs-ci, commit-queue-squash
Commits    5
 - cluster: replace forEach with for..of in lib/internal/cluster/ directory
 - refactor: silence linter
 - build: silence linter
 - fix: handles in a SafeMap
 - Update lib/internal/cluster/primary.js
Committers 2
 - Antoine du Hamel 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/50317
Reviewed-By: James M Snell 
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50317
Reviewed-By: James M Snell 
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 21 Oct 2023 16:56:59 GMT
   ✔  Approvals: 2
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/50317#pullrequestreview-1702814874
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/50317#pullrequestreview-2051249874
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-05-12T07:22:07Z: https://ci.nodejs.org/job/node-test-pull-request/59150/
- Querying data for job/node-test-pull-request/59150/
   ✔  Last Jenkins CI successful
   ⚠  PR author is a new contributor: @jerome-benoit([email protected])
   ⚠  - commit 001629ef3769 is authored by [email protected]
   ⚠  - commit 3deccf496fcb is authored by [email protected]
   ⚠  - commit 0b1f164bdc43 is authored by [email protected]
   ⚠  - commit 92a9ea30040f is authored by [email protected]
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/9053086020
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95
aduh95 merged commit 7b2dc79 into nodejs:main May 12, 2024
@aduh95

aduh95 commented May 12, 2024

Copy link
Copy Markdown
Contributor

Landed in 7b2dc79
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos pushed a commit that referenced this pull request May 12, 2024
@jerome-benoit @targos
cluster: replace forEach with for-of loop
11e9789 
Signed-off-by: Jérôme Benoit <[email protected]>
PR-URL: #50317
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@targos targos mentioned this pull request May 13, 2024
v22.2.0 release proposal #52971
Merged
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
v20.15.0 proposal #53486
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@jerome-benoit @marco-ippolito
cluster: replace forEach with for-of loop
954d2ad 
Signed-off-by: Jérôme Benoit <[email protected]>
PR-URL: #50317
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@jerome-benoit @soophoo
cluster: replace forEach with for-of loop
eb4099f 
Signed-off-by: Jérôme Benoit <[email protected]>
PR-URL: nodejs#50317
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@aduh95 aduh95 aduh95 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. cluster Issues and PRs related to the cluster subsystem. commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. commit-queue-squash PRs the Commit Queue should land as one squashed commit. 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.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@jerome-benoit @TKDev7 @nodejs-github-bot @aduh95 @jasnell
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.