-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
fix: eliminate for ... of syntax #59279
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
base: main
Are you sure you want to change the base?
Conversation
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.
not going to make this ready, looks like some parts of change is unnecessary
const sourceSignalWeakRefs = ArrayFrom(signal[kSourceSignals]); | ||
for (let i = 0; i < sourceSignalWeakRefs.length; i++) { | ||
const sourceSignalWeakRef = sourceSignalWeakRefs[i]; |
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.
That's almost certainly less efficient, ArrayFrom
iterate over the object, so we essentially double the number of iteration
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.
the loop should be converted to a callback, the second argument of ArrayFrom
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 would just leave the for..of
here tbh, but in any case, it should be a separate PR
const encodings = [[kKeyEncodingPKCS1, 'pkcs1'], [kKeyEncodingPKCS8, 'pkcs8'], | ||
[kKeyEncodingSPKI, 'spki'], [kKeyEncodingSEC1, 'sec1']]; | ||
for (let i = 0; i < encodings.length; i++) { | ||
const m = encodings[i]; | ||
encodingNames[m[0]] = m[1]; | ||
} |
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 would try to remove that loop entirely at this point
const encodings = [[kKeyEncodingPKCS1, 'pkcs1'], [kKeyEncodingPKCS8, 'pkcs8'], | |
[kKeyEncodingSPKI, 'spki'], [kKeyEncodingSEC1, 'sec1']]; | |
for (let i = 0; i < encodings.length; i++) { | |
const m = encodings[i]; | |
encodingNames[m[0]] = m[1]; | |
} | |
encodingNames[kKeyEncodingPKCS1] = 'pkcs1'; | |
encodingNames[kKeyEncodingPKCS8] = 'pkcs8'; | |
encodingNames[kKeyEncodingSPKI] = 'spki'; | |
encodingNames[kKeyEncodingSEC1] = 'sec1'; |
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.
yeah, make sense. we could move to a separate PR
let entry; | ||
while ((entry = await dir.read()) !== null) { | ||
const { name } = entry; |
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.
This would need to be evaluated separately
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.
If you make this PR focused on array iteration (i.e. if you remove all the ArrayFrom
calls and leave other non-array iteration for a separate PR), it'd be much easier to review and approve.
I'm +1 on the idea, it also aligns with our recommendation in
node/doc/contributing/primordials.md
Lines 152 to 215 in 9239373
<summary>Avoid for-of loops on arrays</summary> | |
```js | |
for (const item of array) { | |
console.log(item); | |
} | |
``` | |
This code is internally expanded into something that looks like: | |
```js | |
{ | |
// 1. Lookup %Symbol.iterator% property on `array` (user-mutable if | |
// user-provided). | |
// 2. Lookup %Symbol.iterator% property on %Array.prototype% (user-mutable). | |
// 3. Call that function. | |
const iterator = array[Symbol.iterator](); | |
// 1. Lookup `next` property on `iterator` (doesn't exist). | |
// 2. Lookup `next` property on %ArrayIteratorPrototype% (user-mutable). | |
// 3. Call that function. | |
let { done, value: item } = iterator.next(); | |
while (!done) { | |
console.log(item); | |
// Repeat. | |
({ done, value: item } = iterator.next()); | |
} | |
} | |
``` | |
Instead of utilizing iterators, you can use the more traditional but still very | |
performant `for` loop: | |
```js | |
for (let i = 0; i < array.length; i++) { | |
console.log(array[i]); | |
} | |
``` | |
The following code snippet illustrates how user-land code could impact the | |
behavior of internal modules: | |
```js | |
// User-land | |
Array.prototype[Symbol.iterator] = () => ({ | |
next: () => ({ done: true }), | |
}); | |
// Core | |
let forOfLoopBlockExecuted = false; | |
let forLoopBlockExecuted = false; | |
const array = [1, 2, 3]; | |
for (const item of array) { | |
forOfLoopBlockExecuted = true; | |
} | |
for (let i = 0; i < array.length; i++) { | |
forLoopBlockExecuted = true; | |
} | |
console.log(forOfLoopBlockExecuted); // false | |
console.log(forLoopBlockExecuted); // true | |
``` | |
This only applies if you are working with a genuine array (or array-like | |
object). If you are instead expecting an iterator, a for-of loop may be a better | |
choice. |
Inspired from #59278