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

supporting foreach style for-loops #607

Closed
matthewmueller opened this issue Aug 30, 2016 · 15 comments

Comments

@matthewmueller
Copy link

commented Aug 30, 2016

One pattern I use a lot is a for-loop that looks like this:

for (let i = 0, token; token = tokens[i]; i++) {
  // do something with token
}

This errors out, saying:

Expected a conditional expression and instead saw an assignment

I think this is totally appropriate. Unfortunately there's no way to get around this error without changing the loop completely. Here's a syntax that I think should work:

for (let i = 0, token; (token = tokens[i]); i++) {
  // do something with token
}

Since it's explicit that there's an assignment, but it'll give the same error as previously.

@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2016

hah, this feels like such a hack but I quite like it - I might just stop writing Array.forEach if we can make this pass C:

@LinusU

This comment has been minimized.

Copy link
Member

commented Aug 30, 2016

I think it makes sense as well, using parenthesis to indicate that you want assignment and comparison is okay in other places 👌

Also, I think that the following syntax should work in newer runtimes:

for (const [i, token] of tokens.entries()) {
  // ...
}
@matthewmueller

This comment has been minimized.

Copy link
Author

commented Aug 30, 2016

@yoshuawuyts it's great for async/await and generator environments because there's no new async function or function * you need to create.

Just have to be a bit more careful about falsey values. In practice I've only run into that issue like 3 times over the last couple years though.

@jieverson

This comment has been minimized.

Copy link

commented Aug 31, 2016

What about:

tokens.forEach((token, i) => { // do something with token })
@dcousens

This comment has been minimized.

Copy link
Member

commented Sep 1, 2016

for (let i = 0, token; token = tokens[i]; i++) {

Lets hope you're not using numbers, or strings, or anything conceivably falsy...
This is only safe for arrays of objects or arrays of arrays.

@matthewmueller

This comment has been minimized.

Copy link
Author

commented Sep 1, 2016

@jieverson

This approach saves a function allocation and allows you to do this more easily:

for (let i = 0, url; (url = urls[i]); i++) {
  await fetch(url)
}

@dcousens yep:

Just have to be a bit more careful about falsey values. In practice I've only run into that issue like 3 times over the last couple years though.

also, it's perfectly safe for other types when you have control over what you're looping over, which happens quite a bit internally.

@jieverson

This comment has been minimized.

Copy link

commented Sep 1, 2016

Still kind unreadable

@matthewmueller

This comment has been minimized.

Copy link
Author

commented Sep 1, 2016

That's your opinion. Discussing how this code looks and the potential pitfalls of this approach is completely besides the point of this issue.

Please assume that the user knows the pitfalls and doesn't share your opinion on how it looks.

@dcousens dcousens added the question label Sep 1, 2016

@timoxley

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2016

@dcousens unreadable vs unfamiliar perhaps. I think if this were more common it could be parsed with a glance – so long as people don't hide other fancy logic in there.

I've not used this pattern but it's kinda nice in that it removes the crappy extra line to assign a variable to the current iteration in a for i.e.

for (let i = 0; tokens.length; i++) {
  const token = tokens[i] // gross
}

From a quick benchmark on node 6.3.1, performance for this pattern is somewhere between for and forEach. See the ForAlt result below, where each pattern iterates over 10,000,000 items.

For: 186.791ms
ForAlt: 270.132ms
ForEach: 458.686ms
ReduceFunction: 321.777ms
ReduceArrows: 334.990ms
ForOf: 1565.900ms

Discussing how this code looks and the potential pitfalls of this approach is completely besides the point of this issue.

@matthewmueller If I'm not mistaken "how the code looks" and "helping the user avoid pitfalls" are some of the key concerns of standard, right?

@matthewmueller

This comment has been minimized.

Copy link
Author

commented Sep 1, 2016

If I'm not mistaken "how the code looks" and "helping the user avoid pitfalls" are some of the key concerns of standard, right?

@timoxley I don't think it looks any less readable though, in fact more readable than:

for (let i = 0, len = urls.length; i < len; i++) {
  let url = urls[i]
  await fetch(url)
}

also, afaik, you can't make a forEach loop execute serially with async/await or generators, which is where i use this pattern the most. for example, this will execute all the fetches in parallel:

urls.forEach(url => {
  // fail. needs async which makes it execute in parallel
  await fetch(url)
}

I don't think standard should prevent a useful pattern altogether, because there's a potential pitfall. It'll still fail if you just do that outright, but the ( ... ) is more of an "opt-in" to this pattern.

@dcousens

This comment has been minimized.

Copy link
Member

commented Sep 1, 2016

@timoxley to be clear, I never commented on the readability, just the fact it is less safe (as it will early-exit on any falsy elements) and potentially not going to be part of "helping the user avoid pitfalls".

@timoxley

This comment has been minimized.

Copy link
Contributor

commented Sep 1, 2016

Still kind unreadable

@dcousens whoops somehow I read this as your comment, my bad.

@feross

This comment has been minimized.

Copy link
Member

commented Sep 10, 2016

I think it's fine to allow this behavior as an opt-in with explicit parens. We already allow this type of opt-in in other places in standard so this change would be consistent.

@feross

This comment has been minimized.

Copy link
Member

commented Sep 10, 2016

@matthewmueller Are you using the latest version of standard, v8.0.0?

This code lints without any issues for me.

var tokens = []
for (let i = 0, token; (token = tokens[i]); i++) {
  console.log(i, token)
}

Apparently this used to be a bug in an old version of eslint, but it was fixed a while ago.

Your issue should be fixed by upgrading standard.

@feross feross closed this Sep 10, 2016

@matthewmueller

This comment has been minimized.

Copy link
Author

commented Sep 12, 2016

interestinggg. yep upgrading seems to fix it. sorry for the false alarm!

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
7 participants
You can’t perform that action at this time.