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

Disallow for-of #1122

Closed
aboyton opened this issue Oct 13, 2016 · 108 comments
Closed

Disallow for-of #1122

aboyton opened this issue Oct 13, 2016 · 108 comments

Comments

@aboyton
Copy link

@aboyton aboyton commented Oct 13, 2016

The style guide 11.1 says that you shouldn't use for-of and instead should use forEach (which I completely agree with).

That said, the ESLint rule no-iterator doesn't seem to enforce this, and the entry for no-restricted-syntax contains ForInStatement but not ForOfStatement.

Can we add ForOfStatement to no-restricted-syntax (or is the a better way of restricting for-of statements?

@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Oct 13, 2016

Absolutely, thanks! Want to make a PR? it will be semver-major, so i won't be able to merge it immediately, but having it queued up would be great.

@aboyton

This comment has been minimized.

Copy link
Author

@aboyton aboyton commented Oct 17, 2016

Looks like I was beat to the punch by mere hours. Thanks @SimenB.

@dplusic

This comment has been minimized.

Copy link

@dplusic dplusic commented Nov 11, 2016

What should I do in this case:

async function foo(args) {
  for (const arg of args) {
    await bar(arg);
  }
}
@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Nov 11, 2016

@dplusic in nice, simple ES6:

function foo(args) {
  return args.reduce((prev, arg) => prev.then(() => bar(arg), Promise.resolve());
}
@dplusic

This comment has been minimized.

Copy link

@dplusic dplusic commented Nov 11, 2016

It works. Thanks!

@leebenson

This comment has been minimized.

Copy link

@leebenson leebenson commented Nov 18, 2016

@ljharb - what about generators?

I have a function like this that flattens objects recursively, so { user { name: "Lee", location: { city: 'London' } } } becomes {'user.name': 'Lee', 'user.location.city': 'London'}:

function* flatten(obj, str = '') {
  if (obj) {
    for (const name of Object.getOwnPropertyNames(obj)) {
      if (!Array.isArray(obj) && typeof obj[name] === 'object') {
        yield* flatten(obj[name], str ? `${str}.${name}` : name);
      } else {
        yield {
          [`${str}.${name}`]: obj[name],
        };
      }
    }
  }
}

Which is turned into a 'flat' object with Object.assign({}, ...flatten(obj));

I'm probably overlooking an obvious equivalent with map, but a generator solves this quite neatly...

@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Nov 18, 2016

@leebenson generators are not allowed in our guide.

As for an alternative, I'm not sure if this actually works, but perhaps something like:

function flattenObject(obj, prefix = '') {
  return Object.entries(obj).reduce((acc, [key, val]) => {
    const prefixedKey = prefix ? `${prefix}.${key}` : key;
    if (val && !Array.isArray(val) && typeof val === 'object') {
      return Object.assign(acc, flattenObject(val, prefixedKey));
    }
    return Object.assign(acc, { [prefixedKey]: val });
  }, {});
}
@leebenson

This comment has been minimized.

Copy link

@leebenson leebenson commented Nov 18, 2016

Thanks @ljharb. This minor tweak works perfectly:

function flattenObject(obj, prefix = '') {
  return Object.entries(obj).reduce((acc, [key, val]) => {
    const prefixedKey = prefix ? `${prefix}.${key}` : key;
    if (val && !Array.isArray(val) && typeof val === 'object') {
      return Object.assign(acc, flattenObject(val, prefixedKey));
    }
    return Object.assign(acc, { [prefixedKey]: val });
  }, {});
}
@minexew

This comment has been minimized.

Copy link

@minexew minexew commented Dec 5, 2016

@dplusic @ljharb
I don't understand. Where's the advantage in making your code totally unreadable?
Purity is nice, but not when it takes 30 minutes to decipher a simple function.

@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Dec 5, 2016

Of course readability is subjective, but I find loops far more unreadable than a functional alternative.

@minexew

This comment has been minimized.

Copy link

@minexew minexew commented Dec 5, 2016

That's interesting. Of course I don't have any empirical evidence for this, but I bet it'd take the average programer an order of magnitude more time to understand your snippet vs what @dplusic posted.

I hope this fad of functional-at-all-cost is over soon. There's a reason why Haskell hasn't gone mainstream.

@leebenson

This comment has been minimized.

Copy link

@leebenson leebenson commented Dec 5, 2016

I do think @minexew has a point.

With native async/await landing in V8, I think it's going to be pretty common pattern to 'unwind' functional logic like this. for...of and generators often make the code more readable, and easier to follow. I'm not sure effectively banning swaths of valid JS is the way to go.

Personal preference, of course.

@flying-sheep

This comment has been minimized.

Copy link

@flying-sheep flying-sheep commented Dec 6, 2016

i’d actually argue that for..of should be used because of functional style:

.forEach() isn’t functional. it exists for its side effects. so if anything, .forEach() should be banned and for..of allowed in order to make clear that side effects are happening here (which isn’t as apparent from a filtermapforEach pipeline)

@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Dec 6, 2016

@flying-sheep it's a functional evolutionary step to map and friends. for..of does not lend itself towards evolving to be a proper map, filter, etc. In addition, .forEach forces side effects (or else nothing really happens) - for..of can also be used not for side effects.

@flying-sheep

This comment has been minimized.

Copy link

@flying-sheep flying-sheep commented Dec 6, 2016

behold:

function* map(callable) {
	for (const element of this) {
		yield callable(element)
	}
}
function* filter(predicate) {
	for (const element of this) {
		if (predicate(element)) yield element
	}
}

const odd = '12345'
	::map(parseFloat)
	::filter(e => e % 2)

In addition, .forEach forces side effects (or else nothing really happens) - for..of can also be used not for side effects.

both only have side effects (if anything, .forEach returns undefined). what do you mean with “not for side effects”

@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Dec 7, 2016

@flying-sheep lol fair point, both are for side effects since for..of isn't an expression. forget that part.

The :: operator is stage 1, and Airbnb only recommends using stage 3 proposals and above. Similarly, transpiling generators requires regenerator-runtime, which is way too heavyweight to ship to production, so our config also forbids generators at this time.

@SimenB

This comment has been minimized.

Copy link
Contributor

@SimenB SimenB commented Dec 7, 2016

Similarly, transpiling generators requires regenerator-runtime, which is way too heavyweight to ship to production, so our config also forbids generators at this time.

Kinda tangentially related: would you be interested in a config for server-side only projects? I've used airbnb-base at it works fairly fine, but e.g. trailing commas in functions has to be disabled, and generators could be allowed, etc. There's not a lot of rules that needs tweaking, but maybe you could also include https://github.com/mysticatea/eslint-plugin-node.

If you're interested at all, I can create a new issue for it to discuss its merits there? 😄

@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Dec 7, 2016

Server-side only projects should be using babel also.

@SimenB

This comment has been minimized.

Copy link
Contributor

@SimenB SimenB commented Dec 7, 2016

Why? With async/await landing, the only syntax I care about is object spread, and it's OK to use Object.assign. Waiting for trailing function commas until support lands isn't an issue either. Adding the overhead of a build step seems unnecessary.

@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Dec 7, 2016

For one, old syntax is generally faster than new syntax, since it's had time to be optimized. Additionally, there'll always be new syntax you want to use, and if your build process is already in place, it's trivial to add that.

A build step shouldn't add overhead, since you should be running your linter and your tests prior to code going to production anyways - just add the build step into there.

@flying-sheep

This comment has been minimized.

Copy link

@flying-sheep flying-sheep commented Dec 7, 2016

@ljharb maybe i didn’t understand you: you argued that it’s an evolutionary step towards functional array methods. i showed that this isn’t the case. my use of generators is irrelevant. e.g. here’s a map implementation without anything you mentioned:

Array.prototype.map = function map(callback) {
	const mapped = []
	for (const element of this) {
		mapped.push(callback(element))
	}
	return mapped
}
@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Dec 7, 2016

The given implementation of map - or any other abstraction - is entirely irrelevant. forEach is an array prototype method, just like the functional array prototype methods.

@leebenson

This comment has been minimized.

Copy link

@leebenson leebenson commented Dec 7, 2016

I use my babel-preset-node7 for server side use of the airbnb preset. It adds the (very few) missing pieces -- trailing function commas, etc -- and avoids transpiling native features like generators, async/await, etc.

The extra build step makes sense in my case, because I'm writing universal code that targets both the server and the browser, so a build step is necessary anyway. I use generators, async/await, etc freely, and provide a different babel config to each target in webpack. In my use case, adding browser polyfills/runtimes has a negligible effect on the bundle size. My total app is less than 200kb gzipped, with React, Mobx, es2015 polyfills, all app code, etc.

For the most part, I find the airbnb preset to match my own personal stylistic preferences, but I do agree that eliminating for...of is a step too far. I find generators to be very useful for lazy evaluation and for..of (especially coupled with generators and await) makes the code path very easy for others in my team to follow.

In my case, adding a custom rule for no-restricted-syntax fixes my one irritation.

@flying-sheep

This comment has been minimized.

Copy link

@flying-sheep flying-sheep commented Dec 7, 2016

yeah, for..of and generators can be used e.g. for efficient parsers (you delegate to sub parsers via yield*). some languages run on those features extensively, e.g. python and rust.

JS is always the slow kid on the block. maybe in a year or so, we’ll see “map considered harmful” posts that praise how much awesomer generators and comprehensions are.

@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Dec 7, 2016

Comprehensions are never coming to JS, so I doubt you'll ever see that post :-)

@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented May 13, 2018

@riwu that has the same conceptual behavior; the fact that it iterates "unnecessarily" through an array that is almost certainly minuscule in size (less than millions of items) doesn't mean the behavior is significantly different.

@flying-sheep

This comment was marked as off-topic.

Copy link

@flying-sheep flying-sheep commented May 14, 2018

@ljharb you’re still arguing. what do you think about #1122 (comment)?

@riwu

This comment has been minimized.

Copy link

@riwu riwu commented May 14, 2018

transpiling generators requires regenerator-runtime, which is way too heavyweight to ship to production

@ljharb by "heavyweight" do you mean it increases the bundle size?
I noticed the bundle size increased by 7 KB if I use async/await anywhere.
In that case, why doesn't it warn against the use of async/await in general?

Also, why vanilla for loops do not trigger the same warning? Are they acceptable?

for (let i = 0; i < args.length; i += 1) { // no warning triggered here
  await bar(args[i]);  // a different warning (`no-await-in-loop`) is triggered
}
@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented May 14, 2018

@riwu bundle size as well as runtime performance. The guide does disallow async/await in general already, because babel-preset-airbnb doesn’t transform it. No, no loops are acceptable, with or without await, we just don’t have the rules configured to warn on for loops yet.

@SelaO

This comment was marked as off-topic.

Copy link

@SelaO SelaO commented May 15, 2018

Seriously? this:

 function foo(args) {
  return args.reduce((prev, arg) => prev.then(() => bar(arg), Promise.resolve());
 }

is better than this?

async function foo(args) {
  for (const arg of args) {
    await bar(arg);
  }
}

Did it ever cross your mind that you're too strict and you might even be wrong?
You don't see how the first is much less readable, much more complicated and hacky than the latter? Is all the code in Airbnb hacky one liners?

@ljharb

This comment was marked as off-topic.

Copy link
Collaborator

@ljharb ljharb commented May 15, 2018

@SelaO of course, but you're welcome to disagree, and to override any part of the config you like. No, I don't think the latter is more readable - I think the former is the best approach (and in most cases, doing a bunch of async actions in serial is a code smell anyways)

@SelaO

This comment was marked as resolved.

Copy link

@SelaO SelaO commented May 15, 2018

OK. And why does it say here to prefer for-of but I get this with a for-of?

2018-05-15_10-48-36

@ljharb

This comment was marked as off-topic.

Copy link
Collaborator

@ljharb ljharb commented May 15, 2018

@SelaO That says "Prefer JavaScript’s higher-order functions" instead of "loops like for-in or for-of".

@SelaO

This comment was marked as resolved.

Copy link

@SelaO SelaO commented May 15, 2018

Right, but [].forEach() is allowed? How is that better than for-of?

@ljharb

This comment was marked as resolved.

Copy link
Collaborator

@ljharb ljharb commented May 15, 2018

forEach is an iteration, not a loop.

@SelaO

This comment was marked as resolved.

Copy link

@SelaO SelaO commented May 15, 2018

for-of doesn't iterate over all the elements (just like for each but without an unnecessary lambda)?

And if loops are so bad, then why do you allow regular loops then like for(let i = 0; i<x; i++){}?

@ljharb

This comment was marked as resolved.

Copy link
Collaborator

@ljharb ljharb commented May 15, 2018

We don’t yet lint against for loops because we still have them in a number of places. We plan to lint against them at some point too, but for now the general guide advice to avoid all loops is sufficient.

@SelaO

This comment was marked as resolved.

Copy link

@SelaO SelaO commented May 15, 2018

What is the rule to allow for-of? I can't find it.
It looks like no-restricted-syntax is too broad.

@leebenson

This comment has been minimized.

Copy link

@leebenson leebenson commented May 15, 2018

Why are you fighting Airbnb on their own styleguide? If you don't agree with it, extend it. Or go make your own.

I personally use for...of extensively and find it goes hand-in-hand with async/await. I've covered the reasons at length above. But forcing Airbnb to change their own rules to suit your preferences is ludicrous.

@SelaO

This comment has been minimized.

Copy link

@SelaO SelaO commented May 15, 2018

I'm not forcing anyone, I just try to understand or find a good reason for these rules.

@leebenson

This comment has been minimized.

Copy link

@leebenson leebenson commented May 15, 2018

My point is: just extend it and make it your own. Everyone has their own (strong) opinions so trying to change them is pointless.

FWIW, I completely agree that for/of is useful and functional-at-any-cost is overkill. Just do whatever works for your code.

@flying-sheep

This comment has been minimized.

Copy link

@flying-sheep flying-sheep commented May 15, 2018

Why are you fighting Airbnb on their own styleguide? If you don't agree with it, extend it. Or go make your own.

i think your answer is in #1122 (comment)

@leebenson

This comment has been minimized.

Copy link

@leebenson leebenson commented May 15, 2018

i think your answer is in #1122 (comment)

Right, so everything there is to be said, has already been said. Conclusion: Extend/make your own.

linaGirl added a commit to joinbox/eslint-config-joinbox that referenced this issue Sep 6, 2018
I can't figure what's not OK with them in some cases (like when working on async code). Airbnbs argumentation is about immutability. Mine is about readability and ease of use. I like iterators, they are great! See https://github.com/airbnb/javascript#iterators-and-generators and airbnb/javascript#1122 (comment)
@fleksin

This comment has been minimized.

Copy link

@fleksin fleksin commented Oct 25, 2018

Just found this thread. Hope I'm not too late to ask questions.
Say I have a function

someFunc = (array) => {
    for (const item of array) {
       if (someCondition) return false;
    }
    return true;
}

What's the equivalent solution if not using for-of loop ?

@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Oct 25, 2018

@fleksin
Assuming someCondition can better be represented as somePredicate(item), the existing logic gets true if "every item produces false", so:

someFunc = array => array.every(item => !somePredicate(item));

Alternatively, if "any item produces true":

someFunc = array => array.some(item => somePredicate(item));

but you'll want to confirm that the behavior matches your expectations for an empty array.

@hmagdy

This comment has been minimized.

Copy link

@hmagdy hmagdy commented Mar 8, 2019

Just replace for of by .forEach

REPLACE this

async function foo(args) {
  for (const arg of args) {
    await bar(arg);
  }
}

BY this

async function foo(args) {
args.forEach((arg) => {
    await bar(arg);
  }
}

and no need to this weird code

function foo(args) {
  return args.reduce((prev, arg) => prev.then(() => bar(arg), Promise.resolve());
}
@maple3142

This comment has been minimized.

Copy link

@maple3142 maple3142 commented Mar 8, 2019

@hmagdy No, they are not same.
The former executes the promises sequentially, while the latter executes them at them same time.

@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Mar 8, 2019

Indeed, you'd need the (not weird - a normal part of JS and functional programming) reduce instead.

@flying-sheep

This comment has been minimized.

Copy link

@flying-sheep flying-sheep commented Mar 9, 2019

The former executes the promises sequentially, while the latter executes them at them same time.

The former actually results in a “SyntaxError: await is only valid in async function” as expected.

The arrow function isn’t async, and await needs to be in an async function directly. If we make it async, you’re of course right, but that’s not a surprise: Just calling async functions (as forEach does) runs them parallely.


I don’t think this discussion is helpful anymore. Everything’s been said on why the rule is there:

  • The AirBnB JS style is tailored for AirBnB’s use
  • AirBnB targets older browsers without iterator support: #1122 (comment)
  • The AirBnB guide recommends forEach because for..of relies on iterators, and polyfill support for old browsers here is slow.
  • It’s easy to create your own derived style guide that is tailored to your preferences.

I think the only thing left to discuss is what I said in #1122 (comment), and I filed #2019 for that.

@ljharb

This comment has been minimized.

Copy link
Collaborator

@ljharb ljharb commented Mar 9, 2019

To clarify; we recommend forEach because loops should basically always be avoided.

@flying-sheep

This comment has been minimized.

Copy link

@flying-sheep flying-sheep commented Mar 11, 2019

People don’t agree with you here, see the discussion in #2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.