tools: enable ESLint no-var rule in lib #42573

Trott · 2022-04-02T04:20:42Z

First commit is from #42563. Other two commits change the rest of the files in lib and enables the ESLint rule.

nodejs-github-bot · 2022-04-02T04:20:46Z

Review requested:

@nodejs/crypto
@nodejs/http2
@nodejs/net

lib/internal/async_hooks.js

mcollina · 2022-04-02T09:23:39Z

lib/internal/async_hooks.js

-    // Using var here instead of let because "for (var ...)" is faster than let.
-    // Refs: https://github.com/nodejs/node/pull/30380#issuecomment-552948364
-    for (var i = 0; i < active_hooks.array.length; i++) {
+    for (let i = 0; i < active_hooks.array.length; i++) {


I would restrain from making this change unless we have ample evidence that it's as fast as.

@mcollina the for(var...) loop in os.networkInterfaces() was changed to for(let...) in #42563 and I ran a benchmark for that in #42563 (comment) and there was no significant perf regression. Could the results be different for these for loops?

This is one of the hottest loops of the whole Node.js. Keeping a var here does not hurt anybody - most contributors should not even come to this file. Let this be and avoid future problems.

Better to run a benchmark for this PR as well to find out if it's as fast or not. The performance edge cases (no matter the condition, hot or cold paths) were rather fixed in V8 years ago, as also seen in the mentioned PR (#42563).

I'll use comments to disable linting for the ones @mcollina has flagged, and then we can do benchmarks on them or leave them as appropriate without holding up landing the rest of this.

mcollina · 2022-04-02T09:23:44Z

lib/internal/async_hooks.js

-    // Using var here instead of let because "for (var ...)" is faster than let.
-    // Refs: https://github.com/nodejs/node/pull/30380#issuecomment-552948364
-    for (var i = 0; i < active_hooks.array.length; i++) {
+    for (let i = 0; i < active_hooks.array.length; i++) {


I would restrain from making this change unless we have ample evidence that it's as fast as.

mcollina · 2022-04-02T09:23:59Z

lib/internal/js_stream_socket.js

@@ -171,7 +171,7 @@ class JSStreamSocket extends Socket {

    this.stream.cork();
    // Use `var` over `let` for performance optimization.
-    for (var i = 0; i < bufs.length; ++i)
+    for (let i = 0; i < bufs.length; ++i)


I would restrain from making this change unless we have ample evidence that it's as fast as.

Trott · 2022-04-03T17:20:52Z

@mcollina I've reverted the three loops you flagged and instead disabled the lint rule for those lines. PTAL. Thanks!

mcollina

lgtm

nodejs-github-bot · 2022-04-04T10:33:57Z

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

nodejs-github-bot · 2022-04-04T10:53:31Z

Landed in aed18df...b07894d

PR-URL: #42573 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

PR-URL: nodejs#42573 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

PR-URL: #42573 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

PR-URL: nodejs#42573 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

PR-URL: #42573 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

PR-URL: nodejs/node#42573 Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

Trott added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Apr 2, 2022

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 Apr 2, 2022

VoltrexKeyva reviewed Apr 2, 2022

View reviewed changes

lib/internal/async_hooks.js Show resolved Hide resolved

lib/internal/async_hooks.js Show resolved Hide resolved

Trott force-pushed the no-var branch from fb1eafc to 5ff599d Compare April 2, 2022 05:30

RaisinTen approved these changes Apr 2, 2022

View reviewed changes

VoltrexKeyva approved these changes Apr 2, 2022

View reviewed changes

mcollina requested changes Apr 2, 2022

View reviewed changes

Trott added 2 commits April 3, 2022 10:20

lib: prepare files for no-var lint rule

f224401

tools: enable no-var ESLint rule for lib

0a6e197

Trott force-pushed the no-var branch from 5ff599d to 0a6e197 Compare April 3, 2022 17:20

mcollina approved these changes Apr 3, 2022

View reviewed changes

marsonya approved these changes Apr 4, 2022

View reviewed changes

marsonya added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2022

This comment was marked as outdated.

Sign in to view

targos approved these changes Apr 4, 2022

View reviewed changes

Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 4, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 4, 2022

nodejs-github-bot closed this Apr 4, 2022

Trott deleted the no-var branch April 4, 2022 11:05

github-actions bot mentioned this pull request Apr 5, 2022

CI Reliability 2022-04-05 nodejs/reliability#242

Open

13 tasks

This was referenced Apr 5, 2022

V17.8.1 proposal #42612

Closed

v17.9.0 proposal #42613

Merged

github-actions bot mentioned this pull request Apr 6, 2022

CI Reliability 2022-04-06 nodejs/reliability#243

Open

11 tasks

This was referenced Apr 7, 2022

CI Reliability 2022-04-07 nodejs/reliability#244

Open

CI Reliability 2022-04-08 nodejs/reliability#245

Open

CI Reliability 2022-04-09 nodejs/reliability#246

Open

juanarbol mentioned this pull request May 31, 2022

2022-05-XX, Version 16.16.0 'Gallium' (LTS) #43268

Closed

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: enable ESLint no-var rule in lib #42573

tools: enable ESLint no-var rule in lib #42573

Trott commented Apr 2, 2022

nodejs-github-bot commented Apr 2, 2022

mcollina Apr 2, 2022

RaisinTen Apr 2, 2022

mcollina Apr 2, 2022

VoltrexKeyva Apr 3, 2022 •

edited

Loading

Trott Apr 3, 2022

mcollina Apr 2, 2022

mcollina Apr 2, 2022

Trott commented Apr 3, 2022

mcollina left a comment

This comment was marked as outdated.

nodejs-github-bot commented Apr 4, 2022

nodejs-github-bot commented Apr 4, 2022

tools: enable ESLint no-var rule in lib #42573

tools: enable ESLint no-var rule in lib #42573

Conversation

Trott commented Apr 2, 2022

nodejs-github-bot commented Apr 2, 2022

mcollina Apr 2, 2022

Choose a reason for hiding this comment

RaisinTen Apr 2, 2022

Choose a reason for hiding this comment

mcollina Apr 2, 2022

Choose a reason for hiding this comment

VoltrexKeyva Apr 3, 2022 • edited Loading

Choose a reason for hiding this comment

Trott Apr 3, 2022

Choose a reason for hiding this comment

mcollina Apr 2, 2022

Choose a reason for hiding this comment

mcollina Apr 2, 2022

Choose a reason for hiding this comment

Trott commented Apr 3, 2022

mcollina left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

nodejs-github-bot commented Apr 4, 2022

nodejs-github-bot commented Apr 4, 2022

VoltrexKeyva Apr 3, 2022 •

edited

Loading