Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

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

Using 'ForOfStatement' is not allowed (no-restricted-syntax) #1271

Closed
MartialSeron opened this issue Jan 20, 2017 · 153 comments
Closed

Using 'ForOfStatement' is not allowed (no-restricted-syntax) #1271

MartialSeron opened this issue Jan 20, 2017 · 153 comments
Labels

Comments

@MartialSeron
Copy link

@MartialSeron MartialSeron commented Jan 20, 2017

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 ?

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Jan 20, 2017

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.

@ljharb ljharb closed this Jan 20, 2017
@AKST
Copy link

@AKST AKST commented Feb 21, 2017

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.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Feb 21, 2017

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.

@AKST
Copy link

@AKST AKST commented Feb 22, 2017

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.

@petertrotman
Copy link

@petertrotman petertrotman commented Feb 22, 2017

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?

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Feb 22, 2017

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.

@AKST
Copy link

@AKST AKST commented Feb 22, 2017

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...

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Feb 22, 2017

@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.

@AKST
Copy link

@AKST AKST commented Feb 22, 2017

@kouhin
Copy link

@kouhin kouhin commented Feb 23, 2017

@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?

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Feb 23, 2017

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

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

@johannpinson
Copy link

@johannpinson johannpinson commented Mar 1, 2017

@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

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Mar 1, 2017

@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?

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Mar 1, 2017

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

@johannpinson
Copy link

@johannpinson johannpinson commented Mar 2, 2017

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!

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Mar 2, 2017

@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)

@petertrotman
Copy link

@petertrotman petertrotman commented Mar 2, 2017

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.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Mar 2, 2017

@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.

@petertrotman
Copy link

@petertrotman petertrotman commented Mar 2, 2017

@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.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Mar 2, 2017

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.

@petertrotman
Copy link

@petertrotman petertrotman commented Mar 2, 2017

@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 :-)

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Mar 2, 2017

A PR is always welcome, thanks!

@josser
Copy link

@josser josser commented Mar 7, 2017

@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.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Mar 7, 2017

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

@josser
Copy link

@josser josser commented Mar 8, 2017

@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?

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Mar 12, 2017

@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?

@josser
Copy link

@josser josser commented Mar 13, 2017

@ljharb We are usually not using plain generators but async/await.
In #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.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Mar 14, 2017

@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.

@vdh
Copy link
Contributor

@vdh vdh commented Jun 5, 2019

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.

@laurent22
Copy link

@laurent22 laurent22 commented Jul 24, 2019

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.

@vkarpov15
Copy link

@vkarpov15 vkarpov15 commented Jul 24, 2019

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.

@hedleysmith
Copy link

@hedleysmith hedleysmith commented Oct 3, 2019

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

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

@nitsujri
Copy link

@nitsujri nitsujri commented Nov 1, 2019

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:

'no-restricted-syntax': [
'error',
{
selector: 'ForInStatement',
message: 'for..in loops iterate over the entire prototype chain, which is virtually never what you want. Use Object.{keys,values,entries}, and iterate over the resulting array.',
},
{
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.',
},
{
selector: 'LabeledStatement',
message: 'Labels are a form of GOTO; using them makes code confusing and hard to maintain and understand.',
},
{
selector: 'WithStatement',
message: '`with` is disallowed in strict mode because it makes code impossible to predict and optimize.',
},
],

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

@Motoxpro
Copy link

@Motoxpro Motoxpro commented Dec 2, 2019

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.

@vdh
Copy link
Contributor

@vdh vdh commented Dec 3, 2019

@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.

renehamburger added a commit to renehamburger/finanzblick-booster that referenced this issue Jan 5, 2020
See airbnb/javascript#1271 (comment)
and whole thread on why it was disallowed.
@schalkventer
Copy link

@schalkventer schalkventer commented Jan 13, 2020

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 */).

@Arcanorum
Copy link

@Arcanorum Arcanorum commented Feb 3, 2020

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

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Feb 4, 2020

@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.

@BaliBalo
Copy link

@BaliBalo BaliBalo commented Feb 14, 2020

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 #1271 (comment)

@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?

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Feb 14, 2020

@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.

@schalkventer
Copy link

@schalkventer schalkventer commented Feb 15, 2020

@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

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Feb 15, 2020

@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.

@GAUTAMRAJU15
Copy link

@GAUTAMRAJU15 GAUTAMRAJU15 commented Feb 17, 2020

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)
  }));
}

@iwis
Copy link

@iwis iwis commented May 28, 2020

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.

@ljharb
Copy link
Collaborator

@ljharb ljharb commented May 28, 2020

@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.

@iwis
Copy link

@iwis iwis commented May 28, 2020

@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(...).

@ljharb
Copy link
Collaborator

@ljharb ljharb commented May 28, 2020

@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.

@erikeckhardt
Copy link

@erikeckhardt erikeckhardt commented May 28, 2020

@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?

@vdh
Copy link
Contributor

@vdh vdh commented May 29, 2020

@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.

@erikeckhardt
Copy link

@erikeckhardt erikeckhardt commented May 29, 2020

@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...

@vdh
Copy link
Contributor

@vdh vdh commented May 29, 2020

@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.

@nitsujri
Copy link

@nitsujri nitsujri commented May 29, 2020

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.

@airbnb airbnb locked as resolved and limited conversation to collaborators May 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet