Javascript: Using 'ForOfStatement' is not allowed (no-restricted-syntax)

Created on 20 Jan 2017  Ā·  153Comments  Ā·  Source: airbnb/javascript

Hi,
Since everybody tell us to not use forEach() but use for loop instead because it is much faster (https://jsperf.com/test-if-using-forofstatement-is-not-allowed-makes-sens), I would like to know why this is not allowed by airbnb ?

question

Most helpful comment

While I get this is an opinionated linter, the readability of forEach over for of is pretty debatable. But more importantly for of is actually more polymorphic as you can add the iterator property to any data type.

techinically you can add a forEach method to any data type, but šŸ¤·ā€ā™‚ļø

Plus It's also more consistent, as the jQuery implementation of forEach orders parameters differently (yes some people still have to interact with jQuery), that and nodeLists's implementation of forEach is not present on all browsers, where as using for of works once you start using the babel polyfill.

for of is simply a more powerful feature.


For people landing here via google, here's the original definition if you want to over load it.

All 153 comments

a) microbenchmarks (ie, jsperfs) are not accurate reflections of real-world performance
b) performance isn't important. It's easy to make clean code fast; it's hard to make fast code clean.

While I get this is an opinionated linter, the readability of forEach over for of is pretty debatable. But more importantly for of is actually more polymorphic as you can add the iterator property to any data type.

techinically you can add a forEach method to any data type, but šŸ¤·ā€ā™‚ļø

Plus It's also more consistent, as the jQuery implementation of forEach orders parameters differently (yes some people still have to interact with jQuery), that and nodeLists's implementation of forEach is not present on all browsers, where as using for of works once you start using the babel polyfill.

for of is simply a more powerful feature.


For people landing here via google, here's the original definition if you want to over load it.

Array.prototype.forEach.call(nodeList) works in every ES5+ browser.

Separately, jQuery iteration methods should only be used for element collections, so I'm not sure why its element ordering really applies.

You can't yet add the iterator property to any data type, because Symbol is not polyfillable nor available in every browser we support. This guide does not allow using symbols currently.

Array.prototype.forEach.call(nodeList) works in every ES5+ browser.

True, but if the original argument against for of is that it isn't readability, then this isn't a strong argument in favour of forEach.

I'm not sure why its element ordering really applies.

It's less the ordering of the elements and more the ordering ordering of the parameters on invocation, the function you pass to jQuery forEach each passes the index through the first argument and the actual element in the second.Ā 

However if the readability of Array.prototype.forEach.call isn't an issue for you, then this may be a non issue.

There's also the fact you can't yield from a forEach for an outer function the same way you can with a for of, or you use await in an async function (unless this rule accommodates for this).

Symbol is not polyfillable nor available in every browser we support.

Doesn't core-js (used by bable I believe) polyfillSymbol's, as well as well known symbols like the iterator symbol?


edit: Just so I don't waste your time, is there a link to previous discussion on what decisions led to for of being disabled? Because surely stuff like iterating in a generator or async function is something that's been considered.

I'd just like to chime in and say that I would also like a justification for why ForOfStatement is restricted. I would like to use for (let element of array) { ... } in places where I am producing side effects in the iteration.

for(let i = 0; i < array.length; i ++) { ... } is antiquated syntax, and while I know everyone understands what it means, we should be leaving it behind.

array.map has functional connotations and we shouldn't be producing side effects in the closure.

array.forEach is an option, but I personally don't like it for this sort of imperative work.

So I think the ForOfStatement should be removed from the restricted syntax for the above reasons - anyone with any conflicting viewpoints? Do we know what the original justification is?

The primary argument against for..of is that loops are awful, and should always be avoided. forEach is not a loop, it's an iteration.

The primary reason it's disabled in this guide is that Symbols can not be polyfilled, thus we forbid Symbols, thus Symbol.iterator is unavailable, thus for..of is useless in a generic sense.

The primary reason it's disabled in this guide is that Symbols can not be polyfilled, thus we forbid Symbols, thus Symbol.iterator is unavailable, thus for..of is useless in a generic sense.

This thing is though, they are polyfilled... Just look at this bable output of typeof Symbol.iterator, it's pretty clear their not using typeof right out of the box...

@AKST Symbol semantics can not be fully polyfilled, and typeof foo in dependencies will never print out "symbol". It's not a shim, it's a sham.

typeof foo in dependencies

Okay true, fair point. Why this is baked in makes more sense now, cheers

@ljharb I wonder without ForOfStatement, how to loop Map.prototype.keys() and Map.prototype.values(),
e.g.

const map1 = new Map();
// map1.set(someKey, someValue);

for (let key of map1.keys()) {
  console.info(key);
}

map1.keys() and map1.values() are MapIterator. They are non-array and we can't use forEach or map1.keys().length.

Should we convert MapIterator to Array for loop?

@kouhin Array.from(mapOrSet, mapperFunction),Array.from(mapOrSet.keys(), mapperFunction), same for values/entries

in other words, yes - convert it to an array.

@ljharb for...of allow us to use break and continue statement (which can be usefull) but not a Array.map or Array.forEach

What's the solution to simulate this two behavior? (only encapsulate part of code in an if...else and so force to complete the iteration?)

Thanks for your advice

@johannpinson yes, you shouldn't need break or continue ever, that's GOTO. Can you provide a code example with a loop, and I'll try to provide a non-loop example?

Also for anyone struggling to understand the array iteration methods they should be using instead, https://gist.github.com/ljharb/58faf1cfcb4e6808f74aae4ef7944cff may be helpful.

hi @ljharb
I haven't special example which needed it, but i read that for simple loop/iteration of an array, for...of can be use instead of forEach (so only for ES6 / Babel), and also for larger ones, the possibility to break the loop avoid the need to finish the iteration of the array.

This is two simple examples with for..of that i have :

let artistName = ''
for (const artist of data.artists) {
  if (artistName === '') artistName = `<span class="Infos-artist-name">${artist.name}</span>`
  else artistName += `, <span class="Infos-artist-name">${artist.name}</span>`
}

and

database.getIndexes()
  .then((data) => {
    const dates = data.sort((a, b) => b - a)
    for (const date of dates) {
      const li = document.createElement('li')
      li.innerHTML = moment(date).format('ddd<br>DD/MM')
      li.classList.add('Nav-panel-date')
      li.setAttribute('data-date', date)
      li.addEventListener('click', (e) => { this.navTo(e) })

      document.querySelector('.Nav-panel-archive').appendChild(li)
    }
  })

I already use some methods as .map, .filter or .find, but always for...of or for...in for simple loop, often when it doesn't need a return value (like forEach).

An example of the use of break can be with the second code sample.
I retrieve an unkown number of dates, and in the case that i will stop to process it when I exceed the two last month. How can I do it with iteration ?
(Of course, I know that the best solution will be to pass a specific query to the api call that send back directly the good data šŸ˜‰)

Thanks!

@johannpinson

const artistName = data.artists.map(({ name }) => `<span class="Infos-artist-name">${name}</span>`).join(', ');

and

  database.getIndexes().then((data) => {
    const dates = data.sort((a, b) => b - a);
    const listItems = dates.map((date) => {
      const li = document.createElement('li');
      li.innerHTML = moment(date).format('ddd<br>DD/MM');
      li.classList.add('Nav-panel-date');
      li.setAttribute('data-date', date);
      li.addEventListener('click', (e) => { this.navTo(e) });
      return li;
    });
    const panel = document.querySelector('.Nav-panel-archive');
    listItems.forEach((li) => { li.appendChild(li); });
  });

(i'd also recommend using event delegation and adding the click listener onto panel rather than adding it onto every li, but that's out of scope for this question)

This discussion has gone on for a while, and I am convinced that all for of statements could be transformed into Array.forEach statements.

However, it is my opinion that side effects in array iteration is a bad practice and you should be explicit when you are looping over some iterator to produce side effects (e.g. appendChild, Array.push, whatever). In this case, for of has much less of a 'code smell' than Array.forEach. All array methods (for me) have functional connotations that are completely broken by enforcing the use of Array.forEach.

Besides, there is currently no way to distinguish between Array.map and Array.forEach for side effects - you are quite able to do Array.map(el => root.appendChild(el)) without ESLint complaining - even where I'm sure we'd all agree that Array.forEach(el => root.appendChild(el)) is the better alternative. For this reason, for of should be the canonical way of side effect iteration, not Array.forEach, and this should be encouraged by the AirBnB preset.

@petertrotman that's exactly the point. side effecty iterations SHOULD be a code smell. (Array.map and Array.forEach are not functions, but I assume you're using Array as a placeholder here).

Specifically array.forEach((el) => { root.appendChild(el); }) is the better alternative (explicit return, not implicit).

for..of will not be allowed by the airbnb preset because it requires Symbols to exist, and Symbols can not truly be polyfilled - and regenerator-runtime is too heavyweight. This concern outweighs the minor concern that side-effecty iterations (which are rare) are slightly more statically detectable as for..of than as .forEach.

@ljharb Ok, I sympathise with your reasoning. Ideally, I would like two things to result from this discussion:

  1. A clear statement of 'This ESLint preset prefers Array.forEach over for of because the polyfill for for of is too heavyweight for the purpose.' somewhere in the README.
  2. Some way of determining whether Array.map is producing side-effects, and raising an error in this case which demands the use of Array.forEach instead.

I think the first should be ok, but I'm not savvy enough of the internals of ESLint to know how feasible the second part is.

The first has been made, by me, in this thread, multiple times.

The latter is unrelated to the use of forEach vs for..of, because it's a problem with .map. It'd be great to support, but it's impossible to statically know whether .map is called on an array or not.

@ljharb I understand. Still, I think it would be nice for this conclusion to be in the official README - when I searched for the reasoning I found this (closed) issue. I think that, given that this is a 'compromise' solution, it deserves some lines justifying it in the README. I agree with the justification now that you've pointed it out, I just think it is not obvious, and so should be officially made clear.

EDIT: I'll make a pull request for this myself - no reason to expect anyone else to do it :-)

A PR is always welcome, thanks!

@petertrotman there is at least one exception when for..of can't be directly transformed to .forEach / .map:
yield can't be used in map / forEach callback, only in root scope of generator.

@josser yes but this guide forbids using generators, so that isn't relevant to this discussion :-)

@ljharb I know, generators are forbidden because there is no lightweight polyfill for them in browsers.
But in current versions of nodejs we already use them wide.
Does it mean that airbnb is just wrong preset for backend developers?

@josser Although we haven't had the conversation internally yet, I don't think generators are ever a good idea to use, frontend or backend, even if they were trivially polyfillable. Where in node do you find yourself wanting to use them?

@ljharb We are usually not using plain generators but async/await.
In https://github.com/airbnb/javascript/issues/851 I posted example:

for (const model of Object.values(db)) {
    const trigger_version = await db.query('sql get trigger version for ${model}');
    if ( trigger_version =< current_trigger_version)  {
       await db.query('install trigger for sql ${model}');
    }
}

As I understand from discussion in those thread, the reasons of deny generators is same as for async/await. So I'm just called this 'generators' meaning both, generators and async/await.

However, I have interesting idea of using generators in web-scrapping. https://scrapy.org
I know it's python, but I have similar js-code to test this approach and it looks cool.

@josser async function is awesome, and it should be used any time that such use doesn't necessitate regenerator-runtime. await has many pitfalls, but this guide does not yet explain them because on the web, async/await usage still requires regenerator-runtime. In async functions, you do not ever need a loop to do useful things, although certainly using await in a for..of loop seems appealing.

Hi @ljharb

Thank you for your explanation (and sorry for the delay of my response).
I understand better than the use of the Array.func also help to other members of the team to understand what we want to do with the Array (instead of add a comment for each for..of where we can just browse, find, sort or make other operations).

But in the second example, I don't understand well you made a special Array.forEach for the DOM manipulation and don't do it directly in the Array.map ?
Don't hesitate to send me some documentations or reading for explain the behavior/concept used.

I'm also interesting by knowking what about you talk on "side effects in array iteration" with @petertrotman

Thanks

@johannpinson ok so first, Array.map and Array.forEach do not exist - Array.prototype.map exists (commonly denoted as "Array#map"), same for forEach. Regarding the second example in https://github.com/airbnb/javascript/issues/1271#issuecomment-283730081, you'll see that I used map to map one array to another, which has no side effects - but I used forEach for the parts that have side effects. http://softwareengineering.stackexchange.com/questions/40297/what-is-a-side-effect may help you understand "side effects".

How does one use break inside forEach and map? For this reason, I prefer for..of

@binoculars you use some, every, find, findIndex, or you simply don't - break is goto, and that's not good for anyone. If you provide me an example of something you think you need break for, I'll be happy to show you a better alternative.

@ljharb I'm aware of those and use them, usually for the return value. Some and every are objectively slower, because https://es5.github.io/#x15.4.4.17 Lots of type checking going on there. If you have any nested loops, you're creating a function within a function, and that's a perf killer

Performance is the least important thing when coding. If you're not iterating over millions of results, it's not going to matter at all.

Regardless, if you have to optimize, that's when you explicitly violate best practices, because you have profiling and benchmarks to prove you need to.

So the idea is to convert all native iterators to arrays? For instance Array.from(headers.entries())?

@jacobrask that depends on what you need. If your conceptual operation is a "map", then you'd do Array.from(iterator, mapper) and efficiently get an array (just like you'd get from a loop anyways). If what you want is a filter, then you'd indeed do Array.from(iterator).filter(…) or [...iterator].filter(…).

I need a foreach because I want to merge two header objects.

Array.from(extraHeaders.entries()).forEach([k, v]) => headers.append(k, v));

Then yes, that's the proper way to do it, until the Headers API adds something to make merging objects easier.

for..of is also convenient for returning the first found value in a natural way, like this:

for (const item in list) {
  for (const subItem in item.sublist) {
    if (someOtherList[subItem.id]) return someOtherList[subItem.id];
  }
}

We actually have cases like this in our code base, and they are the reason we're disabling this rule. Sure, there are other ways to write this, but is really quite readable.

With Asynchronous Iteration currently in stage 3, the argument against for..of is diminishing.

@janpaul123 that's what .find is for: list.find(item => item.sublist.find(subItem => someOtherList[subItem.id])).

@binoculars The argument remains identical, and this guide/config will not be enabling or allowing async iteration either any time soon.

Async/await is incompatible with foreach. I'm not sure if airbnb is migrating to async/await but the for...of loop produces much cleaner code compared to either a regular for loop or the async map of the array option.

I was wondering what the recommended formatting for iterating an array would be using async/await if for...of is not recommended.

@DevBrent yes, but it's not incompatible with .map and Promise.all, and since async function is just sugar for a promise-returning function, you don't need async/await to do anything cleanly.

If you provide a code example, I'm happy to help show you how to avoid loops.

Following up on @DevBrent / @ljharb – sequential async operations can't use _Array.map_.

Here's an example of a Mocha test:

it(async () => {
  pages.map(async page => {
    await sequentialAsyncOperation();
  });
});

The _sequentialAsyncOperation_'s will be done in parallel, and Mocha won't wait for anything.

Side-note: Bluebird's _Promise.map()_ is useful here.

@johnmanong absolutely they can:

it(async () => {
  return Promise.all(pages.map(async page => {
    await sequentialAsyncOperation();
  }));
});

Bluebird is not spec-compliant (JS-compliant; they are A+ compliant, but that's not as important), so I'd discourage its use.

@ljharb This test runs the async functions in parallel. There are plenty of use cases for sequential async requests. e.g. remote API calls requiring limited concurrency.

@DevBrent easy then:

return pages.reduce(async (prev, page) => {
  await prev;
  sequentialAsyncOperation();
}, Promise.resolve());

@ljharb I think I'd rather make an exception to the rule for the sake of readability.

for (const page of pages) {
  await sequentialAsyncOperation(page);
}

Yep @DevBrent - that's what I did (exception). I just wanted to point out the use case.

This airbnb's rule still bothers people almost a year after this issue has been opened and immediately closed.
So, regarding all the comments above, I'm still not convinced that for...of statements should be avoided.
And, even if I try to use iteration methods most of the time, I added an exception to the rule though.
Thanks anyway for your answer @ljharb

I'm mostly on board with the iteration instead of loops idea, but a loop is more difficult to convert to a non-loop if it's reducing rather than finding. E.g.

let result = { a: { b: {} } }
const keys = ['a', 'b', 'c', 'd']
const defaultResult = null
for (const key of keys) {
    if (typeof result === 'undefined') return defaultResult
    result = result[key]
}
return result

@willclarktech i have no idea what that code does; but here's my attempt at writing it with iterations:

let result = { a: { b: {} } }
const keys = ['a', 'b', 'c', 'd'];
const defaultResult = null;
return keys.reduce((acc, key) => {
  if (acc && acc !== defaultResult) {
    return acc[key];
  }
  return defaultResult;
});

@ljharb It's supposed to be a simplified version of lodash's get: https://lodash.com/docs/4.17.5#get

I.e. follow a nested key path down an object and return the end value, unless any of the keys doesn't have a value, in which case return a default result. reduce seems like the natural iteration to use, but there's no comfortable way of breaking out of it. Short of throwing an error you have to iterate through the entire array even if it's really long and the first key doesn't have a value on the object, so you already know after the first iteration what the final result will be.

How often do you expect the list of nested keys to be long enough where it makes a difference (as in, like a million keys or more)?

Regardless, you can also use .some and mutate a value in a closure, but that's a messy implementation. Either way, I'd suggest not using a loop.

How often do you expect the list of nested keys to be long enough where it makes a difference

Yeah, not often, and I'm struggling to think of an analogous example where you would. For that hypothetical general case though, I guess the proper FP approach would be to use a recursive function though, rather than iteration.

const input = { a: { b: {} } }
const keys = ['a', 'b', 'c', 'd']
const defaultResult = null
const get = (input, [firstKey, ...remainingKeys], defaultResult) => {
    const value = input[firstKey]
    if (typeof value === 'undefined') return defaultResult
    return remainingKeys.length
        ? get(value, remainingKeys, defaultResult)
        : value
}
const result = get(input, keys, defaultResult)

Recursion in JS is rarely well-advised :-)

I still haven't read a good counterpoint to using for .. of to imply you have side-effects. I'm a huge fan of the array methods (my most starred repo here is making a lot of the Array.prototype methods available on PHP), and I totally get not wanting the loops.
That said, sometimes you need the loops. It should be avoided in 99.9% of the cases where I see them used, but I'm not seeing any difference in the readability or pattern itself when you're shoehorning loop logic so that it _technically_ uses an array method.
e.g. this is the lion's share of the few times I need a loop:

// Remove all the scripts from my iframe's DOM
for (const el of iframe.contentDocument.querySelectorAll('script')) {
  el.parentNode.removeChild(el);
}

You can stick it into an array first, but does this really add any clarity? I'd argue I'm obfuscating my own semantics, by implying there's an intent to build a collection when I'm really still just looping:

[...iframe.contentDocument.querySelectorAll('script')].forEach((el) => {
  el.parentNode.removeChild(el);
});

I read the first example, and I'd describe it English as "loop through each of the scripts matched by my querySelectorAll and remove it", but the second one reads like "Spread a list of scripts matched by my querySelectorAll into a new array, loop through that array, and remove each element". So why do I need to think in terms of building a _new_ collection with this array, when I'm still simply running it for the same side-effects?
I do also find it cleaner for async/await. Asyncs help a lot with avoiding heavily-nested code, and keeping the awaits in a loop (when _order_ needs to be enforced) reads naturally from top-down. There are lots of cases where the order to my sequence of promises is important, e.g. animations, API calls, etc.

for (const thingToDo of thingsToDoInOrder) {
  const { message } = await thingToDo();
  displayMessage(message);
}

Promise.all is a great fit, and semantically wonderful, when I'm indicating that concurrency is important. for .. of is clear when sequence is important.

Array.prototype.forEach.call(document.querySelectorAll(ā€˜whatever’), (x) => { x.parentNode.removeChild(x); }); does not create an intermediate array, and the use of forEach clearly indicates side effects.

A loop is simply one (inferior) means of iterating; forEach and friends are not loops.

For promises, reduce over an array of promise-returning functions is the way to indicate serial execution, as is explained multiple times further up the thread.

The Array.prototype.forEach.call only works insofar as something is array-like enough to use it. Ecma's defined a standard iterator via the Symbol.iterator property, so why deliberately run _against_ standards by relying on things being arrayish enough for each function? It's not hard to find iterables that the forEach will fail on:

function* foo() { yield 1; yield 2; yield 3; }
Array.prototype.forEach.call(foo(), v => console.log(v);

but our of will pick up its Symbol.iterator just fine:

for (const v of foo()) console.log(v);

and yes, I know generators aren't well-liked, I'm simply using them as an unambiguously standards-compliant iterable that the forEach.call will fail on. A [...foo()] will, incidentally, use the iterator.
It seems like this code style rule is encouraging people to adhere to practices that the language itself is moving away from.
At very least, it's bizarre that I never see Array.prototype.forEach.call anywhere in the README; in fact, the only Array.prototype invocation anywhere is under the bad section.

@ljharb at very least, will you concede that the README.md, and message given by the rule (iterators/generators require regenerator-runtime, which is too heavyweight for this guide to allow them. Separately, loops should be avoided in favor of array iterations no-restricted-syntax), don't align with the reasoning you've outlined over the past year in this thread? People are understandably confused when their whole reason for not using for .. of is that it's "heavyweight", and aren't given example cases in the official docs.

@jkoudys and this 'heavyweight' is not make any sense in nodejs environment.

@josser it does make sense in a node environment, RAM exists there too - but also, most JS we write is universal, and very little is node-only.

@jkoudys I agree! The readme completely punts on the issue of explaining when (most of the time) and why generators should be avoided, because it's a moot point when generators can't be transpiled and used without performance consequences. We haven't even had the time to write up a section on Promises - something we use and endorse heavily - let alone had the time to write up a section on why we wouldn't use generators even if we could transpile them.

Certainly if the technical obstacle to using generators fades, we'll have to write such a section.

As for Array.prototype.forEach.call examples, that sounds like something that's definitely worth adding. Fwiw, for non-arraylike iterators (of which, absent generators, there are very few), you'd use Array.from and take advantage of its second mapper argument.

In what kind of sense is

return pages.reduce(async (prev, page) => {
  await prev;
  sequentialAsyncOperation();
}, Promise.resolve());

_more readable_ than following?

for (const page of pages) {
  await sequentialAsyncOperation(page);
}

In the only sense that’s possible, subjectively.

@omeid I think it would be a _bit_ simpler than that if you have async/await. You'd need a Promise.resolve() so you could then it if you were using only Promise, but await is nice in that it will return synchronous data too. e.g. 1 === await 1. You could simply omit the second arg and leave it undefined.

return pages.reduce(async (prev, page) => {
  await prev;
  sequentialAsyncOperation();
});

Or am I missing something?

For the record I still think it's silly and convoluted (a reduce is meant to reduce a collection _into_ something, but what I'm I reducing to here? The last Promise? Technically true but a strange thing to grok). await and yield are big reasons in my mind to use for .. of, as they play nicely since they're not inside another function.

Actually now that I've looked through it once, I've changed my mind. It's really not that bad, and we use Arrays with async () all the time to imply a Promise return. Array#map with Promise.all all the time to say 'wait for all these things to happen', so Array#reduce to say 'wait for this sequence to execute in order' is actually pretty reasonable (without starting with a Promise.resolve). Maybe it's just a pattern we need to get used to.

I still like the for .. of for the Symbol.iterator support, of course.

@ljharb The idea that complexity is a purely subjective measure is absurd. The second code snippet reads almost like natural language, where as in the first one, you have to figure it out what is going on.

This style guide was very reasonable but as of recently I am starting to regret having used it for multiple projects, with every upgrade you get more absurd things like making sync programming almost absurd, for example, then you got no-continue and such.

@omeid Complexity is indeed subjective - there's many theories about how to measure it - but we were talking about readability, which is very subjective.

As has been said, async function can't currently be used solely for technical reasons. continue is a separate beast, and blocking it doesn't make sync programming "absurd" at all.

If you dislike any of the parts of this guide, you are certainly free to fork and/or override them.

Complexity and readability go hand in hand, while it is subjective, it is not that impossible to measure it.
And continue does indeed impact code like this:

for (const page of pages) {

  const condition = await shouldAsyncOperation(page)
  if (!condition) continue

  // some other await asyncfunc checks and continues

  await doAsyncOperation(page)
}

vs

pages.reduce(async (prev, page) => {
  await prev;

  await condition = shouldAsyncOperation(page)
  if (!condition) return Promise.resolve()

   // some other await asyncfunc checks and return Promise.resolve()

  doAsyncOperation(page)
}, Promise.resolve())

The second example is using a wrong construct to begin with, we are not reducing pages at all, nor the Promise.resolve() that we pass as initialValue is reduced.

it describes completely different behavior that merely has the same side-effect as our loop. This divergence of Objective and Approach in my opinion objectively dampers readability --readability in the sense of understanding the objective of the code, not merely semantics thereof.

If you dislike any of the parts of this guide, you are certainly free to fork and/or override them.

Jordan, you're certainly the person who has done the hard work to create this project and continues to put work in to maintain it, and I am the guy who is complaining about it, but you have to consider that once you release code for the public, people start to depend on it, and constantly breaking their CI or asking them to change their code isn't going to make anyone happy.

The whole point of using a Code Style is to avoid the never ending discussion around them, so if one has to constantly override rules, they might as well start anew.

I just want to point out that this guideline has been constantly becoming more and more restrictive and requiring overrides, I suggest you to consider that next time new rules are being added; you're of course free to tell me again to _go fork it_, but that is not making good to anyone.

@omeid, your examples are all about doing side effects. To quote the guide:

Don’t use iterators. Prefer JavaScript’s higher-order functions instead of loops like for-in or for-of.

Why? This enforces our immutable rule. Dealing with pure functions that return values is easier to reason about than side effects.

If you want to write a lot of code that relies on side effects, you're going against the spirit of the guide, so of course you will need to manage your own rule overrides for this choice. These issues with the for loops are just the fallout from that decision. Rewriting that logic without the for loops looks bad because the underlying logic is still focused around triggering side effects.

Here are my opinion why i think this rule should be enabled.

  • https://github.com/airbnb/javascript/issues/1271#issuecomment-365328230 about nodeList
  • Using async/await is one of them
  • FormData, Map and anything with else Symbol iterators that don't have forEach
  • Using forEach or other calls often (not always) leads to creating a new anonymes function everytime this can not be optimize by the engine cuz it's recreated everytime.
  • Given this endless (non-arrayish) loop, you can't convert that to an array first and then later do something with it
function* fibonacci() {
  let fn1 = 0;
  let fn2 = 1;
  while (true) {
    const current = fn1;
    fn1 = fn2;
    fn2 = current + fn1;
    const reset = yield current;
    if (reset) {
      fn1 = 0;
      fn2 = 1;
    }
  }
}

You must use for...of in this case or or manually call .next() for..of will be more readable for sure in this case

You argue that it's heavy weights and that you need Symbol and that u require regenerator-runtime but as more and more starts to support it this is becoming the right way of doing things. also if it's only targeted for nodejs on the backend.

@jimmywarting this guide still forbids async/await; Array.from works with anything iterable with or without .forEach; don't make coding choices based on performance, and in fact in v8, a .forEach on an array is now identically as fast as a for loop (just announced).

Do you have an actual example? The fibonacci sequence is an often-used contrived example that has exactly zero bearing on the real world, so it's hard to talk about it meaningfully.

omeid 6 hours ago

ljharb
don't make coding choices based on performance.

No.

Agree....

I don't see anywhere that async/await is forbidden, I'm using it in one work project. (but it could have been enabled also) the only thing i see is using async without any await.

What's wrong with the fibonacci example? Okey if you want some more good usecases:

for...of loops, abrupt iteration termination can be caused by break, continue, throw or return. In these cases, the iterator is closed. and you don't have to go throught all items
You can accomplish task with one loop rather then 2 or more iterations

1) Array.from() iterates over the items and creates a new (potentially giant) array
2) .filter() iterates over the new array, creating yet another (potentially giant) array
3) .slice(0, 5) all that just for 5 elements!
4) .forEach() display

for (const item of items) {
  if (ofInterest) display(item)
  if (enough) break
}

Personally I hate linters. Never dose what i want. I don't use them in any of my open-source projects. I only get frustrated when I use new syntax that isn't supported. You are going to ending up turning off some rules anyway. Linter will always lag behind

If they should do anything It's making the code easier to read & understand. Not forbidding things.

My principle is only:

  • Code it so it looks consistent with the rest of the code.
  • Have somewhere in the back of you head some guidelines, eg like the jsStandard rules

    • Doesn't have to be those or everything they rule out

Otherwise you add more complexity, dependencies, compile time, researching "what is wrong with my code, how to make linters happy" and sending new bug reports to something that is wrong or not supported like when the new async/await/arrow-fn came for example rather then doing productive coding. (like i'm doing now)

But i must admit, Linters is a good tool for the quality of your code. but you must put in some more effort into it. But to me it's not worth it.

I'm pretty sure @ljharb was overly-summarizing the standard refrain that it's easier to optimize clean code than to clean preoptimized code that's messy. In practice you'll find you write much more efficient code by focusing on organisation and readability, rather than trying to shave off picoseconds.

This is a pretty foundational concept of functional programming, with this style guide is built around. It's an excellent guide for an appropriate project, eg a web app. If you're writing something like an encryption algorithm or some network protocol, then you may want to take a more imperative approach, and this guide isn't a good fit.

I don't love for loops, but I also don't see how an Array#forEach is any better, least of all an Array.prototype.forEach.call(.

Maybe the best approach is to leave the rule as-is, that way we have to explicitly eslint-disable around our for ofs. This would make it very clear you're running a loop for its side-effects, instead of building a new array. I'd much rather see comments from someone explaining why the need to loop, than to have devs go through replacing their for loops with forEaches.

If however you did want to replace your foo loops with forEach then you could use the for-each transformation in https://github.com/lebab/lebab

I'm bothered with es lint.
How to rewrite this function?:

handleSubmit = async (event) => {
  event.preventDefault();
  this.setState({ isLoading: true });

  try {
    if (this.state.files.length !== 0) {
      for (const file of this.state.files) {
        await s3Upload(this.ticketId, file);
        this.setState({ message: `Failas ${file.name} sėkmingai ÄÆkeltas` });
        this.snackbar.handleClick();
      }
    } else {
      return null;
    }

    await this.createTicket({
      fields: this.state.newTicketData,
    });
    this.props.history.push('/');
  } catch (e) {
    this.setState({ message: JSON.stringify(e), isLoading: false });
    this.snackbar.handleClick();
    this.setState({ isLoading: false });
  }
}

@Acidfabric do you actually want all those files uploaded one after the other, instead of all at the same time?

If all at the same time:

const { files } = this.state;
await Promise.all(files.map(file => s3Upload(
  this.ticketId,
  file,
).then(() => {
  this.setState({ message: `File ${file.name} was uploaded` });
})));
this.snackbar.handleClick();

If one after the other:

const { files } = this.state;
await files.reduce((prev, file) => prev.then(() => s3Upload(
  this.ticketId,
  file,
).then(() => {
  this.setState({ message: `File ${file.name} was uploaded` });
}))), Promise.resolve());
this.snackbar.handleClick();

@ljharb thank you for the tip!

So if the reason is mostly about not wanting to polyfill regenerator and symbols, what's airbnb's metric for what should be polyfilled?

Going by kangax symols, generators and async functions are supported by every popular browser and the majority of used node versions now.

Is IE support still a hard requirement? Would you be happy with only IE getting the ugly, fat, slow transpiled and polyfilled code, while everyone else gets lean, fast untranspiled code? It's not too hard to do that.

@simonbuchan Symbols simply can’t be shimmed, only shammed (an imperfect polyfilled is often worse than none). regenerator specifically is heavyweight.

IE is still a hard requirement for any global company, but it’s not just IE - there’s way fewer people than devs seem to think that actually update to the latest version of browsers.

Going by caniuse instead (which actually shows usage - select usage relative - I was misreading kangax), that's not really true any more (unless you mean something quite different to me when you say "way fewer"!)
But yes, 15% spread across all those <1% versions is definitely still something to care about. The question still stands of when the cutoff is, and if you would be ok giving them a (slightly) worse experience in exchange for a (slightly) better experience for the other 85%? (And an occasionally much nicer developer experience)
BTW, we've not hit a single problem using the Symbol sham, since it only gets used by generated code and libraries - and both know they could be using a sham and handle it fine - we'd be fine with banning explicit use. Has your experience been different?

@simonbuchan it doesn't matter if you've gotten lucky; incorrect is incorrect. It's going to bite you someday.

Did you see the caveat? Nothing to do with luck.

It's luck that the libraries you're using are handling the specific quirks of the Symbol sham you're using :-)

Not really, because vanishingly few libraries explicitly (eg not via transpilation) use symbols, the ones that do that I've seen all explicitly call it out, and are written to handle sham symbols anyway. Given that you have to work to not work with sham symbols, that's not surprising.

That said, as I said earlier, I am fine with airbnb banning explicit use of symbols, which makes whether a library internally explicitly uses symbols irrelevant!

The much more interesting point is if Airbnb should keep banning language features that use symbols. I'm on the side of the much improved developer experience and possibly performance, but with keeping older platform support that is at the cost of either shipping uglier transpiled code or building multiple bundles.

So that's my current question, I guess, what is the tipping point for airbnb to remove that restriction?

fwiw, we do use features like Array.from and array spread/destructuring, which use symbols (but have fallbacks for the 99% case of "arrays and arraylikes".

The tipping point will be when we don't support any browsers that lack native Symbols - we still support IE 10, for example, so that's likely to be awhile.

Microsoft stop supporting ie < 11 in the beginning of January 12, 2016. That is 2½ years ago..!

The only way to truly embrace modern open standards is to invalidate old technology. So stop supporting old browsers and convince your boss to drop support for IE10 as well.

It's you guys that makes ppl don't want to have to upgrade there browser, leading back to us developer having to deal with it as well.

A platform being EOL is utterly irrelevant; if users are using it, we need to support it, and if users aren't using it, we could drop it even if the vendor still supported it.

Additionally, supporting old platforms and browsers makes upgrading easier for people, not harder - what makes upgrading harder is dropping support, because it increases the risk in an upgrade.

Additionally, supporting old platforms and browsers makes upgrading easier for people, not harder - what makes upgrading harder is dropping support, because it increases the risk in an upgrade.

Just clarifying that this is making Browser upgrades easier for Users, it makes it easy because they won't be afraid of things breaking.

Please don't break the internet for shinny stuff.

I'm happy for my ecma styleguide to support old things. _I_ don't support IE10 or older on any of my sites, but I'd rather that be a decision I can make as the developer, not something I'm forced into by my styleguide.
As much as I like the airbnb style, I've never had a major project that uses it without any tweaks. It's a good, de-facto set of rules, but I'm not going to agree with 100% of the decisions someone else makes for their project. Having 3-4 style overrides using airbnb as a starting point works pretty well, and keeps the style familiar enough that I can onboard new devs without friction.

Any Airbnb guest who tries to book my house using IE10 should be automatically rejected. If they can't figure out how to use Chrome or Firefox they probably can't figure out how to download the August Smart Lock app to get inside my house upon check-in.

@DonkeyAfterDark a) it's not about "figuring it out" - many people don't have the option to upgrade. b) that would violate the spirit of Airbnb's hosting guidelines (and possibly the letter; I'd have to look it up), so I'd encourage you to be more open and accepting of all people.

I keep forgetting Airbnb isn't a JavaScript style guide company šŸ˜…

I stumbled upon this while writing a recursive search function (for a tree structure) resulting in the following code;

const findNode = (node, target) => {
    if (node.id === target) {
        return node;
    }

    for (const child of node.children) {
        const recursiveResult = findNode(child, target);

        if (recursiveResult) {
            return recursiveResult;
        }
    }

    return null;
};

I'm unsure of how to write _efficiently_ (i.e. without iterating through the rest of the tree while the node has been found) without using a return in a while loop. If anybody has a suggestion on how to do this I'd love to know.

The thing about forbidding something like this in a multi-paradigm language like JavaScript is that there are still some problems that are better expressed in an imperative style, both for readability and performance. Also, if given the choice between .forEach(...) and for (let e of elements) { ... }, I'd probably opt for the for...of because the side effect behavior is more explicitly stated, and things like break and continue don't seem to have an analogy in .forEach(...) without workarounds.

Also - the bundle size trade-offs for things like async and the generator/iterator runtime are something that us web developers should be aware of, but we should make that call, depending on project and company - IE11 isn't going to be around forever, and for some companies and projects, support for it is optional.

I think an ideal linter should allow these things, but at least we can override these rules for our own needs :)

@jeffvandyke using .forEach is explicitly stating that it's for side effects, and break/continue is goto - there's better ways to express either.

As for bundle tradeoffs, sure - you can make the call yourself by overriding any rules you like and ignoring any parts of the styleguide that you like. Separately, support for IE 9 and 10 is still not optional in some countries, so it's best to err on the safe side.

@ljharb This thread's getting crazy-long, but you bring up an interesting point (again) about IE support wrt overriding rules. Is there a plan to transition the ruleset to phase out IE9/IE10? eslint-config-airbnb already extends eslint-config-airbnb-base and eslint-config-airbnb-base/rules/strict -- if there were some eslint-config-airbnb/rules/ienineten that had the rules we only need because of

I'd happy to PR on this, at least a first pass, if you have a hunch what all those

@jkoudys we won't be phasing out any browsers as long as airbnb supports them, and potentially for awhile after that.

I don't think there's a simple set of rules for this, though.

How can I enable for of and for in without disabling the more general no-restricted-syntax rule?

"no-restricted-syntax": [0, "ForInStatement", "ForOfStatement"]

Why do I need to go on a quest around the web to find that and it's not in the doc page???

And before you say it @ljharb, no it isn't mentioned here, not the names: "ForInStatement", "ForOfStatement" nor does it say the number should be in the zeroth index.

@SelaO, 'no-restricted-syntax': ['error', 'LabeledStatement', 'WithStatement'], given the default rule location mentioned near the top of this thread.

I got a question. I am sure it will be a face palm for me, but how to rewrite:

sortByObjectProperties(properties = []) {
    return function (a, b) {
      for (const p of properties) {
        if (a[p] < b[p]) {
          return -1;
        } else if (a[p] > b[p]) {
          return 1;
        }
      }
      return 1;
    };
  },

@nue-melexis
I've slightly tweaked it, since by using >/< you're implying numbers (use localeCompare if it's strings), and since the fallback case should return 0, not 1.

sortByObjectProperties(properties = []) {
    return (a, b) => {
      let compare = 0;
      properties.every((p) => {
        compare = b[p] - a[p];
        return compare === 0;
      });
      return compare;
    };
  },

@ljharb

thank you for pointing me.
I can now start rewriting all the for .. of stuff I use :).
This one was the last I did not get yet.

Using .every() to bail at the first false result? That is a little subtle.

A worse performing, but possibly clearer option:

sortByObjectProperties(properties = []) {
  return (a, b) => properties.map((p) => b[p] - a[p]).find((p) => p !== 0) || 0;
},

Also, watch out for non-number values, including undefined and NaN, as using them in a subtraction will give NaN. It's not clear what this should return in that case (probably undefined?), but fortunately - and < / > seem to agree on a > b and (b - a) < 0 in my tests, including '3' < 4, NaN < 3 etc. Just don't assume that !(a < b) && !(a > b) means that a === b.

@nue-melexis i purpose

sortByObjectProperties(properties = []) {
  return (a, b) => {
    const prop = properties.find(p => a[p] !== b[p])

    if (prop == null) return 0;

    if (a[prop] < b[prop]) return -1;

    return 1;
  };
},

You gave me a lesson šŸ¤” ,
will need to dig deeper into it. But nice different solutions.

Thx for you time and brain

@simonbuchan is a human JSCompress. :-D

I might be missing something here, so please excuse me if I'm ignorant, but iterators !== generators and the fact that this warning conflates the two looks bad. I don't see why for/of on an array would require regenerator. You'd just need to polyfill Symbol and convert for (let x of y) to something like let iter = y[Symbol.iterator]; for (let { value, done } = iter.next(); !done; { value, done } = iter.next()) {}. Admittedly I'm not an expert on how transpiling for/of loops works, so I'd appreciate some clarification on that.

I understand that Symbol polyfills are a "sham", but then perhaps the warning should be about Symbols and not about regenerator?

@vkarpov15 that's a fair question. You can't necessarily unroll a for..of loop into a regular loop in a generic fashion; the iterable protocol requires a try/catch, and a few other things.

Certainly polyfilling Symbols requires the warning, but that's a much more minimal issue than needing regenerator - however, even if for..of loops were possible without that transform, they're still loops, and this guide forbids loops.

Then why does the warning mention regenerator at all, rather than indicating that disallowing loops is a purely stylistic choice? I'm not that concerned with the stylistic choice, I'm more concerned that the warning message makes some very wide logical leaps.

The warning includes "Separately, loops should be avoided in favor of array iterations.".

The reason both are mentioned is because indeed, some people avoid the clarity concern (note, this is not merely "stylistic", not that that would make it any less critical), but in general people are (sadly) more responsive to performance concerns.

So what you're saying is that you're stretching the truth in order to make your opinion that this paradigm has better "clarity" seem more legitimate?

@vkarpov15 no, in NO WAY am i stretching the truth; both parts of the warning are 100% accurate. I'm saying that the reason we mention regenerator at all is because even though the clarity argument should be more than sufficient, some people needed the extra rationale.

@vkarpov15 You're being weirdly confrontational?

@ljharb Perhaps swapping the reasons could make your priorities clearer? Currently the message is: iterators/generators require regenerator-runtime, which is too heavyweight for this guide to allow them. Separately, loops should be avoided in favor of array iterations. - it's pretty relevant that for-of transpilation doesn't have anything to do with regenerator-runtime (it's "just" unpacking the iterator and while until done inside a try), so it's weird that this message shows up here (if you could put this on async or function* that would make sense), and double weird it's the first thing you see, when it's not the main reason you're banning this syntax. Separately, (see what I did there šŸ˜‰) I've no idea what "array iterations" is supposed to mean - obviously from context you're referring to array methods like .map(), but the phrase is confusing.

I'd prefer a message along the lines of: Loops are not permitted, prefer array methods such as `.map()`. Avoid using iterators as they have much worse performance in most cases.

Out of curiosity I checked jsperf again, using this microbench instead. I got on my Surface Book 2 15":

test | Chrome 69 | Firefox 61
---- | ---- | ----
for 2 | ~200M | ~380M
for | ~960k | ~1.3M
forEach (both) | ~56k | ~1.2M
for-of | ~430k | ~48k

Weird - that perf difference between for-of and forEach is so huge (in different directions!) it probably swamps whatever transpilation overhead there is?

@ljharb Any idea how I can use an iterator for my usecase? I am running through a series of validation methods for each field config object if a particular verification has been specified and returning the first one that comes up as true. The for...of loop seems great for my situation, so I'd be curious how I could accomplish this with an iteration without running through all the checks each time.

const fieldsConfig = [
  {
    value: email,
    verify: [
      {
        type: 'isPopulated',
        message: 'Please enter your email address',
      },
      {
        type: 'isEmail',
        message: 'Please format your email address correctly',
      },
    ],
  },
  {
    value: password,
    verify: [
      {
        type: 'isPopulated',
        message: 'Please enter your password',
      },
    ],
  },
];

const getValidationErrors = fieldsConfig => fieldsConfig
  .map((field) => {
    for (const verification of field.verify) {
      if (verification.type === 'isPopulated' && !isPopulated(field.value)) {
        return verification.message;
      }

      if (verification.type === 'isGreaterThanLength' && !isGreaterThanLength(field.value, verification.length)) {
        return verification.message;
      }

      if (verification.type === 'isEmail' && !isEmail(field.value)) {
        return verification.message;
      }

      if (
        verification.type === 'isMatched'
              && isPopulated(field.value)
              && isPopulated(verification.matchValue)
              && !isMatched(field.value, verification.matchValue)
      ) {
        return verification.message;
      }

      if (verification.type === 'isCustom' && !verification.condition) {
        return verification.message;
      }
    }
    return false;
  })

// Should return an array like so:
// ["Please enter your email address", "Please enter your password"]

@zeckdude

const getValidationErrors = fieldsConfig => fieldsConfig
  .map((field) => {
    const matched = field.verify.find(verification => (
      if (verification.type === 'isPopulated' && !isPopulated(field.value)) {
        return true;
      }
      if (verification.type === 'isGreaterThanLength' && !isGreaterThanLength(field.value, verification.length)) {
        return true;
      }

      if (verification.type === 'isEmail' && !isEmail(field.value)) {
        return true;
      }

      if (
        verification.type === 'isMatched'
              && isPopulated(field.value)
              && isPopulated(verification.matchValue)
              && !isMatched(field.value, verification.matchValue)
      ) {
        return true;
      }

      if (verification.type === 'isCustom' && !verification.condition) {
        return true;
      }
    ));

    return !!matched && matched.message;
  });

https://gist.github.com/ljharb/58faf1cfcb4e6808f74aae4ef7944cff is a helpful read.

@ljharb This is awesome. So educational! Thanks for taking the time to explain it and for your patience.

Btw, what does GOTO stand for?

@zeckdude ā€œGotoā€ is a colloquial reference to programming styles/languages where one statement can arbitrarily jump execution to any other statement - see https://www.quora.com/Is-GOTO-still-considered-harmful for some more info.

It's also bizarre when developing for Node which supports it natively that you get this error, even with browser: false

@DominicTobias loops are still something this guide prohibits, entirely unrelated to the bundle size argument, in node too.

@zeckdude

Here's my take on that:

const validators = {
  isPopulated,
  isGreaterThanLength: (value, verification) => isGreaterThanLength(value, verification.length),
  isEmail,
  isMatched: (value, verification) => !isPopulated(value)
    || !isPopulated(verification.matchValue)
    || isMatched(value, verification.matchValue),
  isCustom: (value, verification) => verification.condition
};

const getValidationErrors = fieldsConfig => fieldsConfig
  .map(field =>
    field.verify.filter(verification =>
      !validators[verification.type](field.value, verification)
      && validator.message
    )
  );

This could be shortened by making your validator functions have parameters (field, verification) and using them appropriately. isMatched could also check isPopulated on its arguments instead of doing that here (here seems like the wrong place).

I still don't see a clear answer here, or anywhere else, what we should use as an alternative to:

for (const a of arr) {
    await someAsyncFunc(a);
}

where arr should be handled in series.

@AndreasClausen

arr.reduce((prev, item) => prev.then(() => someAsyncFunc(item)), Promise.resolve())

@ljharb That is considerably less readable.

@joemillervi that's a valid but subjective opinion, but of course i'd suggest putting that behind a nicely named abstraction that takes arr and someAsyncFunc.

ā€œSeparately, loops should be avoided in favor of array iterations (no-restricted-syntax)ā€
I agree for 90% of cases, but disagree for:

  1. Breaking out of an iteration early, which forEach, map, filter, reduce can’t do without an exception
  2. Calling await in series on an array of promises
    Currently having issue 1 - I see this was discussed above. My use case is wanting to return early (not waste time doing subsequent iterations) once a match is found.

@joemillervi for 1, some/every/find/findIndex all break early without an exception. For 2, you can do that inside an abstraction with the reduce and a custom rejection.

@joemillervi Maybe this variation is more your style?

await arr.reduce(async (prev, item) => {
  await prev;
  return someAsyncFunc(item);
}, Promise.resolve());

@ljharb It just occurred to me that your example needs an await prefixed to it to be an exact conversion of that pesky for loop.

@vdh it does not (unless it's inside an async function with other code), because it's the return value in a .then, and await is just sugar for a nested promise .then.

The await inside the loop implies it must be inside an async function because without that it's a syntax error. I meant that the for loop will block any code that follows it (and the return promise) in this anonymous async function, but the example substitution will not block, unless it has an await.

I don't see how this is such a good rule when it forces us to go from this:

for (const a of arr) await someAsyncFunc(a);

To this:

arr.reduce((prev, item) => prev.then(() => someAsyncFunc(item)), Promise.resolve())

This is objectively worse: it's longer, it has two levels of nesting, it calls Promise.resolve(), etc. And because it's complicated, it should be wrapped into another function, so another level of nesting.

In my view, this rule fails here because it leads to less readable code just to go around its limitations.

Just worth mentioning - you don't have to use this ESLint preset. I would never consider using this preset again, not because it is bad, but because it is designed first and foremost to help make airbnb eng more effective, and my situation is nothing like airbnb's.

If this rule isn't to your liking, throw the following in an .eslintrc file:

{
  "extends" : [
    "airbnb"
  ],
  "rules": {
    "no-restricted-syntax": 0,
  }
}

If this rule isn't to your liking, throw the following in an .eslintrc file:

{
  "extends" : [
    "airbnb"
  ],
  "rules": {
    "no-restricted-syntax": 0,
  }
}

Specifically this shuts down the whole no-restricted-syntax. If you want to cherry-pick, here is the current definition:

https://github.com/airbnb/javascript/blob/64b965efe0355c8290996ff5a675cd8fb30bf843/packages/eslint-config-airbnb-base/rules/style.js#L334-L352

Since there's no blacklisting, remove:

  {
    "selector": "ForOfStatement",
    "message": "iterators/generators require regenerator-runtime, which is too heavyweight for this guide to allow them. Separately, loops should be avoided in favor of array iterations."
  },

and keep the rest

correct me if I'm wrong, but if each call of someAsyncFunc is independent.

for (const a of arr) {
    await someAsyncFunc(a);
}

can be

await arr.map(a => {
    return someAsyncFunc(a);
});

Which also parallelizes the calls to someAsyncFunc. Like I said, each call needs to be independent.

@Motoxpro You can't await a plain array of promises, you need Promise.all to create a parallel container promise. But a lot of those arguing in favour of the for of syntax seem to want it for a series of blocking side effects.

I am an outsider so this is just my personal opinion, but I don't foresee this rule changing because:

  1. Loops, implicit blocking, and firing side effects are not the style that Airbnb wants for their code. This is their style guide. If you want a different style, override the settings to your preference.
  2. If they do need a blocking series of async side effects, they'd prefer to be explicit rather than rely on implicit syntax. If someone can be confused about whether a loop was intended to be fired in series or parallel, then that is not ideal.

I still use for...of in some rare circumstances (when I want to use continue or break). However, these situations are rare enough that I prefer leaving the default rule and just telling the linter to ignore it on a per individual iterator level if needed for that specific instance (via /* eslint-disable-next-line no-restricted-syntax */).

Should for of be removed from the spec then...?

@Arcanorum nothing people use on the web can be removed; the point of a style guide isn't to decide what should be in the language, it's to decide what subset of the language should be permitted/recommended, and that's what this one does.

Sorry to add to the pile of comments, I agree that for the vast majority of cases map, forEach, find etc. can be used. The only case I came across that wasn't really answered was the one by @janpaul123 in https://github.com/airbnb/javascript/issues/1271#issuecomment-303492127

@ljharb 's answer is not strictly equivalent as it returns the first item that contains the result, whereas the initial code was returning the result value (someOtherList[subItem.id]).

Basically what I am looking for would be some kind of findValue rather than find/findIndex. A find that would return the first returned truthy value. Let's take this simplified example:

function getFirstMatching(array) {
    for (let item of array) {
        const result = heavyTransform(item);
        if (result) {
            return result;
        }
    }
}

The few ways I can think of doing that with Array functions are

array.map(heavyTransform).find(value => value);

or

const resultItem = array.find(heavyTransform);
return resultItem && heavyTransform(resultItem);

Both cases call the heavyTransform function more than necessary.
The only other way to not have extra calls that I can think of seems a bit ugly to me, as I believe the find callback is not supposed to have side-effects (in this case assigning a variable outside its scope):

let result = null;
array.find(item => (result = heavyTransform(item)));
return result;

Is there any other way around that?

@BaliBalo

array.reduce((result, x) => result || heavyTransform(x), null)

For an array of less than, say, millions of elements, the "stop early" performance doesn't matter.

@ljharb Interesting.

Are there sources that you can point me to that goes further into this? I actually prefer to use reduce all around, however, I always assumed that you gain a lot performance-wise by being able to break or continue

@schalkventer performance is both the least important consideration (far behind clarity), and also just doesn’t matter for non-large N. I’m not sure what resources you’re looking for, but you can surely benchmark your entire app with both approaches and see if it makes a difference.

correct me if I'm wrong, but if each call of someAsyncFunc is independent.

for (const a of arr) {
    await someAsyncFunc(a);
}

can be

await arr.map(a => {
    return someAsyncFunc(a);
});

Which also parallelizes the calls to someAsyncFunc. Like I said, each call needs to be independent.

@Motoxpro
If you want to read the files in parallel, you cannot use map indeed. Each of the async callback function calls does return a promise, but you're throwing them away instead of awaiting them. Just use map instead, and you can await the array of promises that you'll get with Promise.all:

async function printFiles () {
  const files = await getFilePaths();

  await Promise.all(files.map(async (file) => {
    const contents = await fs.readFile(file, 'utf8')
    console.log(contents)
  }));
}

The primary argument against for..of is that loops are awful, and should always be avoided. forEach is not a loop, it's an iteration.

The primary reason it's disabled in this guide is that Symbols can not be polyfilled, thus we forbid Symbols, thus Symbol.iterator is unavailable, thus for..of is useless in a generic sense.

We have 2020 year, the Symbol is supported by all modern browsers: https://caniuse.com/#feat=mdn-javascript_builtins_symbol, so Symbol doesn't have to be polyfilled anymore, does it? Does this mean that for ... of can be added to the Airbnb style?

I am still learning JavaScript, so maybe I do not understand something.

@iwis Airbnb still supports IE 11; many websites still need to support IE 9; Symbol can't be shimmed, only shammed; even with Symbols, loops are still not permitted by this guide.

@ljharb If someone doesn't support IE on their website, there is no other problem with using for ... of, is there? For some the for ... of is awful and for others it is more clear than Array.forEach(...).

@iwis there is a problem even if you only support latest Chrome - the issue isn’t a technical one, it’s a conceptual one: don’t use loops, anywhere.

@iwis there is a problem even if you only support latest Chrome - the issue isn’t a technical one, it’s a conceptual one: don’t use loops, anywhere.

A face-off between loops and iterations doesn't seem that useful. Loops are simply a tool—one that can be use to iterate—and thus while I could be missing something, saying absolutely that iteration is superior to loops seems off.

Second, almost anything one might be tempted to call a loopless ā€œpureā€ iteration almost certainly has loops going on under the covers. Taking a stance against loops per se seems like forbidding wheels but accepting machines full of wheels inside. To what benefit to have such a stance rigidly enforced?

@erikeckhardt There's already a long history of comments here on why, but in a nutshell, loops encourage lots of side effects and are often unclear about how async behaviour will flow. If you want a style guide to change how it recommends code style, you're going to need a better argument about code style further than just "it exists".

Arguing about how (one of the many) engines implement the underlying internals is just absurd.

@vdh I guess I’ve just never had any trouble with loops. In my experience people are either confused by async or they’re not. They are either confused by deferred execution or they’re not.

When confused, they inventively find all sorts of ways to do things wrong. Loops or no loops...

@erikeckhardt Style guides are about teamwork and clear communication of intent. Other people need to be able to read and understand your code, and vis versa. It's not just about you, and nothing will be gained from assuming the worst of your peers.

Every reply here will notify everyone subscribed, so if you _have_ something to say it should be something that
a) is not a repeat of the many existing comments above, and
b) is a compelling conceptual argument that does not ignore the project maintainer's existing reasoning he's already explained above.
Anything else is just rude and creates extra noise.

I vote to lock this thread as this is no longer a discussion and hasn't been for some time.

Whether you believe forEach to be right or wrong it's okay. AirBnB has their opinion and if you don't agree with it, it is not forced on you - there is a way to not use it.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

josmardias picture josmardias  Ā·  37Comments

AdamBrodzinski picture AdamBrodzinski  Ā·  36Comments

ljharb picture ljharb  Ā·  139Comments

alexfedoseev picture alexfedoseev  Ā·  76Comments

aboyton picture aboyton  Ā·  113Comments