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

[francoisromain]

Since version 8, there is this error on for in loops.
What should be used in place of for-in loops ?
Thanks

[softwarespot]

I was literally thinking the same 20 minutes ago and couldn't see any mention of this in the README.

[ljharb]

https://github.com/airbnb/javascript#iterators--nope

Use things like map/every/some/filter/reduce/find/findIndex etc to iterate over arrays, and Object.keys/Object.values/Object.entries to produce arrays so you can iterate over objects.

Feel free to provide some loops (ideally concrete use cases and not contrived examples), and I'll happily show you a better way to write the code with iteration instead of a loop.

[softwarespot]

Of course, but maybe this should be added to the README under Objects?

[ljharb]

PRs are welcome to improve the documentation.

[francoisromain]

@ljharb thank you very much

[francoisromain]

@ljharb

my loop was like this:

for (const i in options) {
    if (opts && opts.hasOwnProperty(i) && options.hasOwnProperty(i)) {
        options[i] = opts[i];
    }
}

and it is now like this:

Object.keys(options).forEach((key, index) => {
    if (opts && opts.hasOwnProperty(key)) {
        options[key] = opts[key];
    }
});

is there a better way?

[ljharb]

@francoisromain Object.assign(options, opts)

[francoisromain]

@ljharb oh wow! : )

[francoisromain]

@ljharb it's not really the right place to ask, here is an unrelated question:

Is there a better syntax for this one?

if (value) {
  options.value = value;
}

Thank you

[ljharb]

@francoisromain no, that is the proper way to do it, so that you only perform an assignment in the case where the conditional is true.

[francoisromain]

@ljharb ok thanks

[sveip]

Another one for you @ljharb :)

let count = 0;
const selected = this.state.selected;
for (const wanted in selected) {
  if (selected.hasOwnProperty(wanted)) {
    if (selected[wanted] === true) {
      count++;
    }
  }
}

[ljharb]

@sveip

const count = Object.values(this.state.selected).filter(Boolean).length;
/* or, if you really need to only check for literal true */
const count = Object.values(this.state.selected).filter(x => x === true).length;

[jchbh-duplicate]

if use for-in/ for-of with async generator function, how to change it?

Promise.coroutine(function*() {
  const files = ['f1', 'f2', 'f3']; 
  for (let file of files) {
     yield fs.readFileAsync(file);
  }
})

By the way, this code can be replace into async-await style in ES7 syntax.

[ljharb]

I don't know what Promise.coroutine is, since that's not part of standard Promises (and JS can't truly have actual coroutines).

For the body of the generator though, Promise.all(files.map(file => fs.readFileAsync(file))) gives you a promise of the results if you want them read in "parallel". Certainly if you want generator semantics, you need for..of there, but since our styleguide discourages use of generators…

[EvanCarroll]

@sveip

Or, with _.reduce:

let countTrue = ( input, selected ) =>
    _.reduce(input, (result,v,k) => { if(selected[k] === true) {result++} return result; }, 0 );

countTrue( {a:1,b:2,c:3,d:4}, {a:true,b:true,z:true} ); // 2
countTrue( {a:1,b:2,c:3,d:4}, {a:true,b:false,z:true} ); // 1

[EvanCarroll]

There are a few instances where for...in statements are useful, here is one. If you're using generators for async code (as in the case of Co.js and Koa),

You can do this..

Object.keys(file.data.languages.data).forEach( key => {
  // stuff with access to key...
} );

That works, but now if you want to make it async this won't work.

Object.keys(file.data.languages.data).forEach( key => {
  yield stuff();
} );

You can go back to this..

for (key in file.data.languages.data) {
  if ( !file.data.languages.data.hasOwnProperty(key) ) { continue; }
  yield stuff();
}

[kesne]

It's worth noting that the styleguide doesn't allow generators, so if you're overriding that rule for you project, you could also override the for-in rule.

[EvanCarroll]

Yea, well not allowing generators was all kinds of silly being that they're the easiest path to upgrading to async/await, and babel is a pain in the rear for development.

[ljharb]

Even with generators, you never want for..in with objects. You'd do for (const key of Object.keys(obj)) { yield stuff(key, obj[key]); }

[mhofman]

So what is one supposed to do if they want to iterate over all enumerable properties of an object, not only their own properties? I don't see any solution besides a very cumbersome Object.getPrototypeOf loop and remembering which properties were already visited to avoid revisiting "overridden" properties.

The problem with the no-restricted-syntax rule is that to re-enable the for ... in restricted syntax locally, you have to re-emit the rule with all the other restricted syntax statements minus the ForInStatement, which is very error prone.

[ljharb]

@mhofman
I agree that overriding this rule is annoying, as I've had to do it in a couple modules that build abstractions around for..in.

I'm curious, what's your use case for wanting inherited properties?

In general, the thing you want is an ES spec proposal: Object.enumerable{Keys,Values,Entries}, but it doesn't exist yet. There used to be Reflect.enumerate but that's now removed.

[mhofman]

My use case is a logging library that enumerates over all properties to log them. I agree it's pretty rare to actually have to use for ... in but the lack of alternatives right now makes its syntactic disabling very annoying.
I thought the guard-for-in was a pretty good way of enticing people to avoid using it, while still allowing its use when necessary (easy rule to disable).

[ljharb]

guard-for-in just requires any conditional check - often it false-negatives on places where people aren't checking "own property", precisely to allow the use case you mean.

If eslint added a more specific way to block for-in loops or an easier way to override it, I'd be all for that - but imo the annoyance of overriding it for a small handful of edge cases is worth the benefit of never having to see a for-in loop anywhere else :-)

[dandean]

@ljharb – I love this thread. I'm hoping I can pile on. If you have an object and you want to remove all keys with a value of null, undefined, or '', but not other falsy values what would you do instead of for..in? This is what we have:

for (let param in query) {
  if (
    query.hasOwnProperty(param) &&
    query[param] === null ||
    query[param] === '' ||
    query[param] === undefined
  ) {
    delete query[param];
  }
}

[ljharb]

@dandean

Object.entries(obj).reduce((acc, [key, value]) => {
  if (value || typeof value === 'number' || value === false) {
// or: if (value !== null && value !== '' && typeof value !== 'undefined') {
    acc[key] = value;
  }
  return acc;
}, {});

You definitely don't want to use delete.

[dandean]

Ah dang, that requires Object.entries which I'm not using in this project yet.

[ljharb]

@dandean https://npmjs.com/object.entries ; but sure:

Object.keys(obj).reduce((acc, key) => {
  const value = obj[key];
  if (value || typeof value === 'number' || value === false) {
// or: if (value !== null && value !== '' && typeof value !== 'undefined') {
    acc[key] = value;
  }
  return acc;
}, {});

[coljung]

Jumping on the band wagon, I was hoping to get this verified:

for (const k in cloneKeys) {
    if ({}.hasOwnProperty.call(cloneKeys, k)) {
        Object.keys(cloneProcessors[k]).forEach((itm) => {
            if (itm !== 'id' && itm !== 'position') {
                delete cloneProcessors[k][itm];
            }
        });
    }
}

I modified it to:

Object.keys(cloneKeys).forEach((key, index) => {
            if ({}.hasOwnProperty.call(cloneKeys, key)) {
                Object.keys(cloneProcessors[key]).forEach((itm) => {
                    if (itm !== 'id' && itm !== 'position') {
                        delete cloneProcessors[key][itm];
                    }
                });
            }
        });

But i still find it could be further improved.

Thanks in advance!

[EvanCarroll]

@coljung Object.keys returns just that object's keys. It doesn't climb up .prototype so the .hasOwnProperty call isn't needed.

Object.keys(cloneKeys).forEach(key => {
                Object.keys(cloneProcessors[key]).forEach(itm => {
                    if (itm !== 'id' && itm !== 'position') {
                        delete cloneProcessors[key][itm];
                    }
                });
        });

But, I mean even that seems a bit silly because you're simply deleting all items on cloneProcess that aren't id or position. So with lodash.pick, assuming the cloneProcess[key] is ok with the default object prototypes.

Object.keys(cloneKeys).forEach(key => {
                Object.keys(cloneProcessors[key]).forEach(itm => {
                        cloneProcessors[key][itm] = _.pick(cloneProcessors[key], ["id","position"])
                });
        });

You can also examine ES6 Map if you want to move forward a bit and it works for your needs.

[prontiol]

Here's my case:

returnSuccessfulFn() => {
  for (const fns of fn) {
    const result = fn()
    if (result) {
      return result
    }
  }
  return null
}

1. I need to return null (not undefined) if none of the fns returned non-falsy value
2. I need to return the result of the fn, not the fn itself
3. Each fn must be called no more than once

[ljharb]

@prontiol assuming fns is an array:

function invokeFind(arr) {
  let lastResult;
  arr.some((fn) => {
    lastResult = fn();
    return lastResult;
  });
  return lastResult || null;
}
invokeFind(fns);

[prontiol]

One more:

for (const [key, value] of Object.entries(obj)) {
   ...
}

[ljharb]

@prontiol Object.entries gives you an array, so you can just .forEach or .map etc over it. There's never a need for a loop :-)

[prontiol]

In general, the only problem I have with .map, .reduce, .forEach, etc instead of for loops is: you can't just return the result at some point of iteration.
I mean, you can't do something like this:

function processItems(items) {
  for (const item of items) {
    const result = item.generateResult()
    if (isValid(result)) {
      return result
    }
  }
  return null
}

because inner return in Array iterator methods will only return from the inner function in iterator, but not from the outer function. I know, it looks like old plain C, but the javascript version:

function processItems(items) {
  let validResult
  items.some(item => {
    const result = item.generateResult()
    if (isValid(result)) {
      validResult = result
      return true
    }
  })
return validResult || null

looks even worse!

1. you have to declare an intermediate result variable validResult, which is not needed in for version
2. you have to do variable assignment + return true value, instead of just returning the value
3. you have to do final check validResult || null
4. you have to use (and ignore the result) semantically wrong Array.some just to be able to interrupt the loop
5. in nested loops things get far more complicated with tracking those saved results and making sure you interrupted all the loops correctly, I mean:

for (const a of as) {
   ... some code
   for (const b of bs) {
      ... some code
      if (c) {
        return a + b + c
      }
     ... some code
  }
  ... some code
}

but the last I guess can be solved with re-writing your code and splitting it into smaller functions, so that's not a big deal, but still for is the winner here.

[ljharb]

@prontiol there's some, every, find, and findIndex that can return early, all of which can be wrapped with your own abstractions. For example, you could make mapFind which takes an array, a mapper, and a finder, and returns the first mapped value that passes the finder predicate.

The point is that _your code_ should not have loops in it - if you use a good abstraction, it doesn't matter what it's implemented with - even if it's implemented with a loop, or with a "semantically wrong" array method.

[joaocunha]

This thread is really cool. Thanks for all the pointers, @ljharb.

Can I ask you what you usually do for NodeLists?

[ljharb]

@joaocunha a NodeList is arraylike, so it'd need a for statement, never for..in - however Array.prototype.map.call(nodeList, x => x.tagname) etc should work just fine.

[joaocunha]

@ljharb would you recommend against a spread operator? I find it cleaner this way.

const selectElements = document.querySelectorAll('select');
[...selectElements].forEach((select) => {
  // ...
});

[ljharb]

@joaocunha that works just fine! However, it creates an extra array, whereas the .call avoids that.

[felixsanz]

Note: I'm posting this here becuse i'm getting Using 'ForOfStatement' is not allowed (no-restricted-syntax). So it's the same after all.

TL;DT: No need concurrency? Using await? --> for..of is KING and other things like map or reduce are PAINFUL.

---

@ljharb for..of is really handy when you have to await inside. If you use .map you have to use await Promise.all(arr.map(async)) all the time. And if you have to use await several times inside, the map version breaks separation of concerns (or it's just too hard to manage).

Example. (i hope this dummy example is not too stupid).

Note: I know map + Promise.all is concurrent and for..of executes one by one. I used map to simplify the example. The real function to compare here would be .reduce((p, v) => p.then(v), Promise.resolve()) (something like that).

  const articlesFinal = new Map()
  for (const { title, body, metadata } of articles) {
    articlesFinal.set(await slugify(title), {
      body: await parseMarkdown(body),
      metadata: await someFn(metadata),
    })
  }

If you want to achieve the same using .map you have to:

Break separation of concerns:

await Promise.all(
  Object.entries(articles).map(async ({ title, body, metadata }) => {
    // Apply all awaits here.
    // So this map is not separating concerns and it's doing all transformations inside.
    // As a one single function.
  })
)

You go out of variable names:

const slugified = await Promise.all(
  articles.map(async (article) => ({ ...article, title: await slugify(article.title) }))
)

const parsedMarkdown = await Promise.all(
  slugified.map(async (article) => ({ ...article, body: await parseMarkdown(article.body) }))
)

const articlesFinal = await Promise.all(
  parsedMarkdown.map(async (article) => ({ ...article, metadata: await someFn(article.metadata) }))
)

See what I mean? You have to create temporal variables and it's really hard to name those things. For no benefit.

---

In case someone is going to reply with something like this:

const articlesFinal = await Promise.all(
  articles
    .map(async (article) => ({ ...article, title: await slugify(article.title) }))
    .map(async (article) => ({ ...article, body: await parseMarkdown(article.body) }))
    .map(async (article) => ({ ...article, metadata: await someFn(article.metadata) }))
)

No. Thats not going to work. Every map functions returns a promise and the next on the chain can't take any value.

[leebenson]

@felixsanz, there's a discussion on for..of in #1122

[ljharb]

@felixsanz Using await inside a loop means you're forcing serial execution when you want parallel execution - Promise.all is INCREDIBLY more efficient, and that is exactly and only what you should be using there.

[felixsanz]

@leebenson thanks. i'll take a look and continue there.

@ljharb In this case i intentionally want serial execution. Sometimes there is nothing wrong with it (CPU-intensive task, lot of elements to process). In this case you don't want Promise.all right? Timeouts and other stuff involved.

[leebenson]

@felixsanz - Promise.all if the async methods can run independently of each other (and you only care about resolving when they're _all_ done), serial execution if you need to work with a resolved value before moving on.

Example: Inserting a bunch of independent records to the DB is likely faster with Promise.all. Whereas needing the previous record's _id_ field to insert a related join probably needs to happen sequentially.

[felixsanz]

@leebenson If you have a CPU-intensive task to apply to every element on an array, and that array is big, doing it with Promise.all is just a bad idea. Or if it uses lot of memory, doing it like that will crash the processes and hang the system

[ljharb]

Simply having a big array is a code smell. let alone doing CPU-intensive tasks. Obviously if you have those things, you can use for..of and a lazily computed iterable, but that requires cooperation in the entirety of that stack - in other words, that's a special case, aka an exception. Exceptions shouldn't be permitted by a style guide; that's what rule override comments are for.

[tracker1]

There's an issue with this, in that you aren't able to use Async/await or generator/yield with an iterator...

for (var option of options) {
  await processOption(option);
}

I suppose, that I could do something like

await Promise.all(options.map(processOption));

But that means I can't do certain things as cleanly, and some instances it makes things much more ugly

[ljharb]

Does processing each option require that processing the previous option be completed? Is there a dependency on the first processOption call in the second one?

If not, then for..of is in fact a horribly wrong way to express it, and the Promise.all variety is actually the superior approach.

Either way, this guide forbids loops, so for..of shouldn't pop up. Feel free to post a new issue if you think there's a use case for loops that this guide interferes with.

[tracker1]

I understand.. for my two uses, I'm able to adjust to adjust to map/forEach, but there have been times in the past where I specifically didn't want to do too much at once.

[ljharb]

Happy to entertain discussion of those in a new issue if it comes up again.

[MaxSvargal]

ForOf iterator needed if you use generator's yield or await.

[ljharb]

@MaxSvargal a) this guide forbids generators and async functions atm, so that's a nonissue. b) you don't need for..of even with generators, since you can use [...generator()] and then use iteration methods; c) this guide's linter config forbids using await in a loop (even if you could use for..of or async functions)

[Willibaur]

@ljharb Awesome thread before hand.
Could you please help me with this one, thanks in advance

  _markSteps: function (index) {
    let steps = this.get('steps');
    for (let [i, value] of steps.entries()) {
      if (i <= index) {
        Ember.$(`.${value}`).addClass('active');
      } else {
        Ember.$(`.${value}`).removeClass('active');
      }
    }
  },

[felixsanz]

@Willibaur Just use forEach

[ljharb]

@Willibaur

  markSteps: function (index) { // note: do not use underscores to falsely mean "private"
    const steps = this.get('steps');
    steps.forEach((value, i) => {
      if (i <= index) {
        Ember.$(`.${value}`).addClass('active');
      } else {
        Ember.$(`.${value}`).removeClass('active');
      }
    });
  },

[Willibaur]

Thanks @felixsanz and @ljharb for your help

[prontiol]

@ljharb What's wrong with using underscore?

[ljharb]

@prontiol it conveys a false sense of privacy - underscored properties are fully public, and need to be treated that way. The guide has a section on it.

[prontiol]

@ljharb We generally use underscore prefix to tell the minifier to mangle names of these functions.
What's the alternative?

[ljharb]

@prontiol can you elaborate on the minifier you use where this is even an option, and how it's configured? My personal preference is to not mangle any function names; Airbnb currently mangles them all.

[prontiol]

@ljharb I use UglifyJS2 (as a WebPack plugin)
Here's the sample config:

new webpack.optimize.UglifyJsPlugin({
    ...<some non-relevant options>
    mangle: {
        props: {
            regex: /^_/,
        },
    },
}),

In my case bundle size matters, so I am trying to mangle everything I can, but my library implements public interface, so it must keep specific names of some methods.

[ljharb]

@prontiol that's interesting, thanks! In this case I'd choose a different convention, primarily because a leading underscore tends to mean "i really wish this was private" - and that's not what you're trying to convey. Maybe /^%_/ or something, i dunno :-)

[hechukwu]

@ljharb hey, what will be a better version for this

try {
  for (const book of fileData) {
    count++;
    for (const key in book) {
      this.strip(book[key]).forEach((word) => {
        if (!terms.hasOwnProperty(word)) {
          terms[word] = [];
        }
        if (terms[word].indexOf(count) > -1) {
          return;
        }
        terms[word].push(count);
      });
    }
  }

[ljharb]

@andela-hchukwu That's missing a bit of the code (initializing count and terms, for example) but assuming fileData is an array and book and terms are normal objects:

let count = 0;
const terms = fileData.reduce((acc, book) => {
  count += 1;
  return Object.entries(book).reduce((acc, [key, value]) => {
    return this.strip(value).reduce((acc, word) => {
      if (!Array.isArray(acc[word])) {
        acc[word] = [];
      }
      if (!acc[word].includes(count)) {
        acc[word].push(count);
      }
      return acc;
  }, {});
});

I'm not sure why the try/catch is there, I named three things "acc" (for "accumulator") and they could use better names, and I'm missing a lot of context (which makes naming hard), but I'd do something like that, split up into many more smaller functions.

[hechukwu]

Thanks @ljharb for your response, count was initialized, here was the full code though

  /** creates an inverted index
   *
   * @param {String} fileName name of file to be indexed
   * @param {String} fileData data of file to be indexed
   * @return {Object} data to be indexed
   */
  createIndex(fileName, fileData) {
    const terms = {};
    let count = 0;
    if (typeof (fileData) !== 'object') {
      return 'invalid json file';
    }
    try {
      for (const book of fileData) {
        count++;
        for (const key in book) {
          this.strip(book[key]).forEach((word) => {
            if (!terms.hasOwnProperty(word)) {
              terms[word] = [];
            }
            if (terms[word].indexOf(count) > -1) {
              return;
            }
            terms[word].push(count);
          });
        }
      }
    } catch (e) {
      return { error: 'invalid json format' };
    }

[josser]

Another example (pseudocode):

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

I can't use Promise.all because it will hurt db instance with a lot of sql queries running them simultaneously, so I want them executed one-by-one.

[felixsanz]

@josser Something like this:

Promise.all(Object.values(db).reduce((p, model) => p.then(() => {
  const trigger_version = db.query('sql get trigger version for ${model}')

  if ( trigger_version =< current_trigger_version)  {
     return db.query('install trigger for sql ${model}');
  }

  return Promise.resolve()
})), Promise.resolve()))

[josser]

@felixsanz no, db.query is async :)

[felixsanz]

@josser Well, you didn't use await :) (you edited the message after i posted mine)

Simply add a promise to db.query then.

[josser]

@felixsanz yep, sorry. I've used it in second case but forget in first.
What did you mean by 'add a promise to db.query'?

[felixsanz]

@josser

Promise.all(Object.values(db).reduce((p, model) => p.then(() => db.query('sql get trigger version for ${model}').then((trigger_version) => {
  if ( trigger_version =< current_trigger_version)  {
     return db.query('install trigger for sql ${model}');
  }

  return Promise.resolve()
}
))), Promise.resolve()))

[josser]

@felixsanz
I thought, main idea of eslint and presets is to simplify developers life by making code more readable and understandable.
So, you think this is more readable and easy-to-understand version than mine?

[felixsanz]

@josser You can improve readability on this version, it was just an example, not a copy-paste solution.

[ljharb]

@josser if your query stuff was properly encapsulated in a separate function, you'd have Promise.all(arrayOfPromiseReturningFunctions.reduce((prev, task) => prev.then(task), Promise.resolve())) - which is quite simple and readable, and conveys your intention far more succinctly than a loop does.

[justinbowes]

Set#values returns an iterator function, and for-of iterates that trivially.

I can think of two alternatives, both markedly inferior to for-of:

• iterating the set twice (once for Array#from or spread, and once to iterate) or
• iterating using for or while, manually calling next() and checking .done

Any workaround?

[ljharb]

@justinbowes Set and Map both have .forEach, if you want to iterate them only once using their default iterator (values for Set, entries for Map). You'd want to use Array.from with the mapper argument if you were doing a map operation, because that's both functional and efficient.

In no case would you want a loop; iterating twice is much better than using one.

[justinbowes]

Missing Set#forEach was a significant oversight on my part. Thanks for the pointer.

[BlueRaja]

for..in enumerates properties in the prototype chain, Object.keys() does not. What is the replacement in the case where that actually matters?

[ljharb]

@BlueRaja in the rare use cases where that actually matters (what's yours? i'm only aware of "spec-compliant ES5 polyfills" most of which I've written, and "ill-informed inheritance approaches" which are obsolete now that there's class), you have two options: use for..in, but buried deep inside a well-tested abstraction, or better, recurse using Object.keys and Object.getPrototypeOf. I'd recommend using https://npmjs.com/protochain and iterating over that array rather than recursing or using a while loop.

[BlueRaja]

(My use-case is super-obscure: a work-around for a bug in the Firefox Gecko-Selenium driver when attempting to return an XHRPromise via browser.execute)

[ljharb]

@BlueRaja for something that oddball, i'd say just use an eslint override comment around your for..in :-)

[halvardos]

@ljharb I found a less obscure use case for this. If you get the validity of an input field and want to iterate over the values to figure out what, if any, values are invalid you get an object where the parameters are stored up the protochain.

const validity = document.getElementById('#someInputField').validity;
Oject.keys(validity); // []
Object.keys(Object.getPrototypeOf(validity)); // (11) ["valueMissing", "typeMismatch", ...]

[ljharb]

@halvardos indeed, that's a new one. I'd still recommend putting that code in a separate function, and using an eslint override comment there, since the real best practice there is to explicitly iterate those keys and not to reflect on them (in case new ones get added in the future).

[Andriy-Kulak]

Hi @ljharb! Any idea on how to improve below? submissionsArray is an array of objects. In the loop below, I am adding some additional key/values to it and pushing every newly created object into a const list.

    const list = []
    for (let item of submissionsArray) {
      const premiumType = item.rating[`${item.type}`]
      list.push({
        ...item,
        primaryInsuredName: item.primaryInsuredName,
        totalCost: item.totalCost ? formatDollars(item.totalCost) : 'n/a',
        quotedPremium: (premiumType && premiumType.premium) ? formatDollars(premiumType.premium) : 'n/a'
      })
    }

[ljharb]

@Andriy-Kulak That's a straightforward map.
``js const list = submissionsArray.map((item) => { const premiumType = item.rating[${item.type}`]
return {
...item,
primaryInsuredName: item.primaryInsuredName,
totalCost: item.totalCost ? formatDollars(item.totalCost) : 'n/a',
quotedPremium: (premiumType && premiumType.premium) ? formatDollars(premiumType.premium) : 'n/a'
};
});

[grundmanise]

Hi @ljharb , is it a proper way to use forEach here?

Was

    result = {};
    for (let prop in params) {
      result[`${key}[${prop}]`] = params[prop];
    }

Now

    result = {};
    Object.keys(params).forEach((prop) => {
      result[`${key}[${prop}]`] = params[prop];
    });

[ljharb]

@grundmanise yes! Although it'd be even better like so:

const result = Object.entries(params).reduce((acc, [prop, value]) => ({
  ...acc,
  [`${key}[${prop}]`]: value,
}), {});`

[TwoAbove]

Well, there is a specific instance where it is super convenient to use a for of loop (With Node 8 goodness):

const asyncFunction = async (element) => { ... };
const array = [...];
const doAsyncConsecutively = async (array) => {
    for (const element of array) {
        await asyncFunction(element);
    }
}
doAsyncConsecutively(array)

[leebenson]

@TwoAbove, that syntax is partly what led to the discourse. There are limited use cases for this, though- more often than not, you want I/O to run at 'full speed' by allowing async code to fire at will.

[TwoAbove]

@leebenson Yes, you're correct there. That's what exceptions are for - adequate decisions for tasks at hand. I added the comment because I did not see anything similar in this thread, and thought it might be useful for people looking for something similar and wondering why their code gets flagged

[fvgs]

@TwoAbove you can express the consecutive behavior with the following:

const consecutive = array.reduce((promise, element) => (
  promise.then(() => asyncFunction(element))
), Promise.resolve());

[leebenson]

I don't disagree with you @TwoAbove. In fact, I posted praise for exactly this syntax in #1122, and argued for it at length. I actually run into this scenario often.

But I take @ljharb's comment on board too that it's often abused, without thinking about what's really happening under the hood. Awaiting a Promise before running another Promise that could probably have run quickly in 'parallel' (or at least fired before the other one finished, and then resolved with Promise.all()) is _generally_ the more useful.

For times when sequential execution of Promises is wanted, I agree that for..of is syntactically nicer. Despite being contrary to this styleguide.

Edit: And here, specifically.

[TwoAbove]

The last link is golden, @leebenson 👍

I think what really skews the view, is that in airbnb-base's (not sure about the others) no-restricted-syntax rule, the ForOfStatement is marked as an Error, although it's _mostly_ just a tip that usually means cleaner intentions and better code overall.
If you take into account ForInStatement, WithStatement, and LabeledStatement, which have some gotchas or clunky behavior associated with them, ForOfStatement stands out.

I do understand that there were right intentions with setting it as an Error, but were there talks about making it a Warning?

Just curious 😄

[leebenson]

It's probably an error because Airbnb expressly forbid generator syntax in any form - it being a 'warning' probably doesn't fit their use case.

In any case, the best thing to do is simply to override with your own config. Then you side-step syntactic politics altogether.

I do that in my ReactQL starter kit with plenty of rules.

The lines that side-step ForOf are:

// ...
const [_, ...restricted] = baseRules.rules['no-restricted-syntax'];

// ...

{
  rules: {
    'no-restricted-syntax': [2,
      ...restricted.filter(
        r => !['ForOfStatement'].includes(r.selector)
      ),
    ],
  },
}

// ...

FWIW, my preferred styleguide is now something I like to call 'Airbnb+' - not to everyone's cup of tea, of course, but it works for me: https://reactql.org/docs/styleguide

[TwoAbove]

That eslint config looks frighteningly familiar, with some things a bit different 😅

So not to flood this issue, yes, airbnb doesn't like some js features. They have their own reasons for it. But everyone has a head on their shoulders, and nobody ever forbid reasoning and adequate solutions, so if you just for yourself that using generators, for of loops or labels will be beneficial (although I'm curious about the last one), nobody can stop you from doing it.

And I'm going to use you stylesheet as a base @leebenson - it's great, and sure is my cup of tea, thanks

[ljharb]

Warnings are useless; if it doesn't block merges to master, people will just ignore it.

As always, you're free to override any rules you like, or to fork the guide entirely.

[tjwudi]

I think this shouldn't be an ERROR.

Many of us develop & use ESLint on existing repositories which may use for...in extensively. It doesn't make any sense to require people to not to use a totally valid, backward compatible syntax. I agree map/reduce/filter are apparently cool ways to write code but this doesn't establish the conclusion that for...in is a style ERROR.

[TwoAbove]

@tjwudi You are correct, but we need to remember one specific detail: this is airbnb's eslint config, and they use it with their repos in mind, not the other way around.

Would you change your preferred style because somebody's project is using syntax that you do not want to use, but they do?

[leebenson]

@tjwudi - just extend Airbnb's config. What's the big deal?

[ljharb]

@tjwudi it absolutely should be an error. If you're using a codebase full of legacy practices, you should be doing the work of either adding eslint override comments, or fixing the violations.

In the meantime, you can (as has been pointed out) turn the rule off in your eslint config.

Also, nothing is "just style". Every single thing that eslint, and this guide, legislates is something that might be subjective style to some, but has caused bugs for others - including something as simple as whitespace choices.

[tjwudi]

Great thinkings folks. Here are my two cents, I want to run through these comments one by one.

"this is airbnb's eslint config, and they use it with their repos in mind, not the other way around." -- @TwoAbove

Sure, agree. Two things:

1) My question really was "should this be an error". So do you think using "for ... in" is an error?
2) This is a successful and outstanding open source project. It's good to keep in mind that this is designed in Airbnb's context but this is not a reason we don't bring up new ideas to make it fit into more projects.

just extend Airbnb's config. What's the big deal? -- @leebenson

I'll quote @ljharb, "Warnings are useless; if it doesn't block merges to master, people will just ignore it.".

Same here, errors are useless, I can just extend it :/

Again my question really was: "should this be an error".

Every single thing that eslint, and this guide, legislates is something that might be subjective style to some, but has caused bugs for others -- @ljharb

Appreciate your thought, and I totally agree.

Don't get me wrong, I love functional programming styles. I use it extensively in the daily job and loved them. But things have their downsides, sometimes there are things you can achieve with a simple for loop but you refactored it to functional styles then people are confused.

"ERROR" is a very strong word, it means "MUST NOT" in RFC 2119. I personally agree with your ideas but is this thing really "ERROR"-worthy? I request a second thought from you.

[ljharb]

@tjwudi yes, I consider for..in a "must not" error. There is only one use case in the language for which it's required; which is inside a spec-compliant shim for various ES5 methods - in these places, an eslint override comment is appropriate (the es5-shim has a number of these). Additionally, when utterly hidden inside a clear abstraction, and if performance reasons absolutely require that your benchmarked, tested, clear, working code be faster, and for..in is provably faster, then go for it, with an eslint override comment. Anywhere else? It should be written as clearly as possible without using for..in.

My opinion on for..of, for loops, and while loops are slightly different, and are largely due to my belief that loops are inherently bad style that makes code harder to maintain (please open a new issue if you wish to debate that; this thread is long enough). for..in, however, is buggy across browsers in lots of subtle ways even I couldn't fully enumerate, and is always better replaced by usage of Object.keys or other object enumeration API methods.

[TwoAbove]

@tjwudi, Thanks for your two cents.
As @ljharb mentions, there are good reasons for avoiding for of loops (and some other js features or formatting).
Although it might seem silly at first, why would such a rule exist? Is it really worthy to be an error? We can think of it this way: this must have bitten them once (at least), and forced them to waste time (which is golden) on some debugging that could have been avoided with a rule. Is it worth ignoring and making the code more readable in some places and saving time when somebody is examining the code (warning or ignoring), or saving time on debugging if something goes wrong (error)?

I suppose we can boil it down to a simple question: what will save you more time (and nerves) in the long run?

It's for you to decide, using common sense and good judgement.

[tracker1]

@ljharb Here's an example...

async main() {
  const todoList = await getList(); // returns 2174 items
  for (let item in todoList) {
    await processItem(item);
    if (global.gc) global.gc();
    // processItem, downloads a tarball from the url in item
    // extracts N number of separate CSVs
    // parses each CSV and imports it into dynamo
  }
}

Quite literally, one wouldn't be able to handle two thousand simultaneous runs of this process... perhaps you can suggest an alternative that doesn't use for-loop, that isn't more ugly? Or won't cause node to crash a horrific death when it runs out of memory.

[ljharb]

@tracker1 you'd need the expose-gc flag for node to be able to do that; and it's generally not a good idea to write JS with control of the garbage collector - so your example is already far beyond best practices.

What I'd suggest is using an appropriate data structure - ie, a stream or an Observable - rather than a Promise (which is supposed to be for a single value; an iterable of promises isn't the same thing as "multiple future values"), such that you can operate on a chunk at a time, and you'd never need to worry about memory pressure.

[kyawkyawsoezhu]

  shapeCategoriesOption(categories) {
    const result = [];
    for (const category of categories) {
      result.push({ name: category.name, id: category.id });

      if (category.sub_categories) {
        for (const subCategory of category.sub_categories) {
          result.push({ name: subCategory.name, id: subCategory.id, className: 'sub-category' });
        }
      }
    }
    return result;
  }

How can I improve and not using for of?

[TwoAbove]

@kyawkyawsoezhu Let's not necro this. You can use reduce and forEach

shapeCategoriesOption = categories => categories.reduce((result, category) => {
  result.push({ name: category.name, id: category.id });
  if (category.sub_categories) {
    category.sub_categories.forEach(subCategory => result.push({ name: subCategory.name, id: subCategory.id, className: 'sub-category' }));
  }
}, []);

@ljharb Maybe close comments here? (If that's an option) I don't think there will be a good discussion

[ljharb]

I don't mind continued questions about how to rewrite loop code into superior non-loop code; I'll certainly lock the thread if comments become nonproductive.

[kyawkyawsoezhu]

@TwoAbove thanks for your answer, but I got error said TypeError: Cannot read property 'push' of undefined and eslint warning message for reduce Expected to return a value in arrow function

[ljharb]

@kyawkyawsoezhu try this:

shapeCategoriesOption(categories) {
  return categories.reduce((acc, category) => (
    acc.concat(
      { name: category.name, id: category.id },
      (category.sub_categories || []).map(subCategory => ({
        name: subCategory.name,
        id: subCategory.id,
        className: 'sub-category'
      })),
    );
  ), []);
}

[kyawkyawsoezhu]

@ljharb It's awesome, thanks

[technowar]

@ljharb

How can I clean this block of code

      let argumentsCount = 0;

      Object.keys(args).forEach((key) => {
        if (Object.prototype.hasOwnProperty.call(args, key)) {
          if (argumentsCount) {
            path += '&';
          }

          path += `${encodeURIComponent(key)}=${encodeURIComponent(args[key])}`;
        }

        argumentsCount += 1;
      });

expected result would be foo=bar&baz=qux.

[josser]

@technowar just use qs.stringify()

[karol-majewski]

@technowar How about:

Object.entries(args)
  .map(([key, value]) => encodeURIComponent(key) + '=' + encodeURIComponent(value))
  .join('&');

[technowar]

@josser, thank you but I prefer using vanilla.
@karol-majewski, neat. Thank you!

[ljharb]

@technowar https://github.com/airbnb/javascript/issues/851#issuecomment-322518110 is a fine functional approach, but you absolutely should never prefer "vanilla" over using a proper lib (NIH is a cancer), and there's a number of issues that could happen when constructing a query string that qs fixes for you - so this guide's official position on query strings is to use qs.

[technowar]

Understood. Thank you @ljharb.

[SakoMe]

What would be the best way of fixing this?

        const data = [];
        for (const key in response.data.query.pages) {
          data.push(response.data.query.pages[key]);
        }

[ljharb]

@SakoMe const data = Object.values(response.data.query.pages);

[sveip]

A lib I'm using has alot of these:

function convertTagAndFieldKeys(data) {
  const result = [];
  if (!data || !data.results) {
    return result;
  }
  _.forEach(data.results, (item) => {
    _.forEach(item.series, (series) => {
      const columns = series.columns;
      const values = _.map(series.values, (arr) => {
        const tmp = {};
        _.forEach(arr, (v, index) => {
          const name = columns[index].replace(/tag|field/, '').toLowerCase();
          tmp[name] = v;
        });
        return tmp;
      });
      result.push({
        name: series.name,
        values,
      });
    });
  });
  return result;
}

Will it increase performance to refactor them? And if so, any suggestions on how?

[ljharb]

Performance is the least important concern. It might make them more readable; that should be the motivation. One issue with using lodash iteration methods is that it’s reslly hard to know if you’re iterating over an object or an array - it makes it hard to refactor away from them.

[masatoTokyo]

@ljharb

How could you clean this?

  const str = [];
  for (const p of Object.keys(obj)) {
    if (Object.prototype.hasOwnProperty.call(obj, p)) {
      str.push(`${encodeURIComponent(p)}=${encodeURIComponent(obj[p])}`);
    }
  }
  return str.join('&');

I tried to make it clean myself but it returns something I don't expect.

  Object.keys(obj)
    .map(([p, value]) => `${encodeURIComponent(p)}=${encodeURIComponent(value)}`)
    .join('&');

[ljharb]

@masatoTokyo that's because Object.keys gives you an array of keys; for your .map to work you'd need to use Object.entries. Otherwise it's correct.

[heisian]

Webpack 4:

Migrate to using for of instead of forEach

Performance!

[ljharb]

@heisian I'm not sure what you mean (or how Webpack 4 is relevant); for..of is still banned by this guide, and because of the iterator check, it's not actually going to be more performant.

[heisian]

What is the iterator check?

[ljharb]

for..of uses the iterable protocol, so for (const k of o) {} has to do const iterator = o[Symbol.iterator](); and then iterator.next() for each item; simple array iteration just walks up the indexes.

[sveip]

Can I ask for some clues on how to convert _only_ the "numbers" (that currently are strings) to numbers in an object without a foreach?

Input:

{ outerObj: 
  { firstElement: '1',
    secondElement: 'banana',
    firstElement: '123',
    firstElement: 'peanut' } }

Desired output:

{ outerObj: 
  { firstElement: 1,
    secondElement: 'banana',
    firstElement: 123,
    firstElement: 'peanut' } }

[ljharb]

@sveip here’s a first attempt:

function stringifyIfNumeric(value) {
  if (typeof value === ‘string’) {
    const num = parseInt(value, 10);
    if (!isFinite(num) && String(num) === value) {
      return num;
    }
  }
  return value;
}
function convertNumerics(obj) {
  return Object.entries(obj).reduce((acc, [key, value]) => ({
    ...acc,
    [key]: value && typeof value === ‘object’ ? convertNumerics(obj) : stringifyIfNumeric(value),
  }), {});
}
convertNumerics(input);

This was typed on mobile, so i haven’t ran it and there might be minor mistakes, but hopefully you get the gist.

[sveip]

@ljharb, fantastic, thanks. Will try it out. (massively impressed by your mobile keyboard coding/typing skills!👍)

[sveip]

Ok, so I gave it a go, but there is something I dont fully understand.

The Object.entries(obj) transforms the obj

{ outerObj: 
  { firstElement: '1',
    secoundElement: 'banana',
    thirdElement: '123',
    fourthElement: 'peanut' } }

to a list of lists like so:

[ outerObj, 
  { firstElement: '1',
    secoundElement: 'banana',
    thirdElement: '123',
    fourthElement: 'peanut' } ]

then the reduce returns objects that are spread over the acc object, (?) but the first reduce has the current values

acc:  [ 'firstElement', '1' ]  
key:  secoundElement 
value:  'banana'

second:

acc:  undefined 
key:  thirdElement 
value:  123

and so on.

[ljharb]

@sveip not quite. Object.entries returns an array of "entry" arrays, which are two-item arrays of key and value. So, the list of lists is more like [['firstElement', '1'], ['secondElement', 'banana'], ['thirdElement', '123'], ['fourthElement', 'peanut']].

The first reduce returns an object literal in each iteration, so acc should never be undefined. You may want to read https://gist.github.com/ljharb/58faf1cfcb4e6808f74aae4ef7944cff just to be sure we're on the same page about how reduce works :-)

[sveip]

@ljharb, thanks. I was probably not that clear, but what i wrote above is what node v9.6.1 returns given your code above and my input from the example. Might it be that node has a different Object.entries?

[ljharb]

@sveip no, it definitely doesn't, but as i said, I wrote that code on mobile so it might not work as-is :-)

Try this:

function stringifyIfNumeric(value) {
  if (typeof value === 'string') {
    const num = parseInt(value, 10);
    if (isFinite(num) && String(num) === value) {
      return num;
    }
  }
  return value;
}
function convertNumerics(obj) {
  return Object.entries(obj).reduce((acc, [key, value]) => { console.log(key, value); return ({
    ...acc,
    [key]: value && typeof value === 'object' ? convertNumerics(value) : stringifyIfNumeric(value),
  }); }, {});
}
const input = {
  outerObj:  {
    firstElement: '1',
    secoundElement: 'banana',
    thirdElement: '123',
    fourthElement: 'peanut',
  },
};
convertNumerics(input);

(I had two bugs; I was passing obj instead of value into the recursive convertNumerics call, and stringifyIfNumeric was returning the number if it was not finite, instead of when it was finite)

[sveip]

You are absolutely right, sorry about that. In my response with the code I made a typo, the inner array should have been an object, I'll correct that to avoid any confusion (although it most likely only confused me...)

Thanks allot, I will read your "array iteration thoughs", I for sure have quite some learning to do :+1:

[coldner]

@ljharb how about this?

for (let word of dictionary) {
await new Word(word).save();
}

[leebenson]

Promise.all(dictionary.map(word => new Word(word).save()))

[coldner]

@leebenson
console.time('promise');

await Promise.all(dictionary.map(word => new Word(word).save()));

console.timeEnd('promise');

console.time('for-of');

for (let word of dictionary) {
await new Word(word).save();
}

console.timeEnd('for-of');

Output:
promise: 7314.911ms
for-of: 2533.061ms

tested it few times, similar results

[leebenson]

How many words do you have in the array?

Could be that you're exhausting I/O running them concurrently, and it's quicker to run sequentially.

Try a Promise throttler. Here's a simple one I wrote recently. You could run, say, 10 at once, and find that sweet spot for what's a sensible default in your app.

[coldner]

Imho it's bad to have too more dependencies in app... especially, when there is a native code which you can use to achieve something, easier than with external dep

[leebenson]

Do whatever works for your app. IMO, there's nothing fundamentally wrong with for/of and sequentially awaiting a Promise, if that's indeed what you're intending to do.

However, for most use-cases, it's an unnecessary bottleneck and will slow you down. It's nearly always faster to send, say, 10 concurrent network requests and let the event loop take care of scheduling them vs. explicitly waiting for one to finish before executing the next.

Re: deps - nothing stopping you creating a throttler in your own code. It'll crunch/gzip down to a few bytes.

[EmilyRosina]

@ljharb :wave: how about...

const aliases = {
  '@': 'src',
  shoelace: path.resolve(__dirname, 'src/components/shoelace'),
}

function resolveSrc(_path) {
  return path.join(__dirname, _path)
}

function registerWebpackAliases() {
  const webpackAliases = {}
  for (const [alias, aliasPath] of Object.entries(aliases)) {
    webpackAliases[alias] = resolveSrc(aliasPath)
  }
  return webpackAliases
}

[ljharb]

@EmilyRosina

return Object.fromEntries(Object.entries(aliases).map([
  alias,
  aliasPath,
] => [
  alias,
  resolveSrc(aliasPath),
]));

(See https://npmjs.com/object.fromentries if you need to polyfill it)

[lincolnaleixo]

@ljharb Is it possible to convert a function that has async sleep?

for (const postLink of postLinks) {
  await likePost(postLink);
  await sleep(3000);
}

PS:
const sleep = milliseconds => new Promise(resolve => setTimeout(resolve, milliseconds));

[karol-majewski]

@robot110011

postLinks
  .reduce(
    async (previousPromise, post) => {
      await previousPromise;
      likePost(post);

      return sleep(3000);
    },
    Promise.resolve(),
  )

[lincolnaleixo]

Wow thanks @karol-majewski ! Not good to read tough

[lincolnaleixo]

Is this loop acceptable by the guide?

for (let index = 0; index < postLinks.length; index += 1) {
  await likePost(postLinks[index]);
  await sleep(3000);
}

[ljharb]

@robot110011 there's no "sleep" in JS; i'd name that delay. but i'd do this:

postLinks.reduce((prev, post) => prev.then(() => likePost(post)).then(() => sleep(3e3), Promise.resolve());

All loops are discouraged by this guide.

[lincolnaleixo]

Really appreciate the lesson! Cheers

[crobinson42]

Early returning in a loop is the use case for using for in and for of, ie:

for ( const task of taskList) {
  // return from loop
  if (someCondition === someTestCondition) return

  // else continue iterating in loop
}

[ljharb]

@crobinson42 for those use cases, it's better to use (or build) an iterating abstraction like .some or .find etc, so that your intention is clear. Often this can mean, breaking the loop body up into multiple iterations, if it's conceptually doing multiple tasks (like a map + find)

[rodrigoabb]

It might be worth to notice: there are things that can't be (easily) done with Object.keys/Object.values/Object.entrieswith a foreEach()as suggested. For example, if we use a filter, we will need to return true or false inside. As it's said here forEach() cannot be stopped, so we can't return/break. We could throw an exception though, but it's a really bad practice.

[ljharb]

@rodrigoabb some/every/find/findIndex short-circuit, ftr. show me some code that you think can't be easily done with iteration methods, and i'll be happy to supply you with code that is.

[joaocunha]

This thread is already 3 years old and you're still teaching and helping people out with their iterations. You're such a good sport, @ljharb.