Remove for of loop from sim code

[zepumph]

It looks like babel transpiles this into using Symbol and expects a polyfill. We don't do polyfill that right now. On top of that @jonathanolson says that forEach loops are faster even with the closure. As a result we should convert usages to that, and when necessary use the traditional for (let i=0; i<length; i++).

I see 75 usages of for \( let \b[A-z]*\b of, though many can stick around because they aren't in sim code. Assigning to responsible devs of these repos.

• friction
• wave interference
• fractions-common
• gravity force lab
• Enumeration

[samreid]

See also phetsims/wave-interference#223

[samreid]

I recommend to add a lint rule so this syntax doesn't accidentally sneak in. However, once we introduce a lint rule for this, we will need to address the 55 other occurrences of for...in that occur in non-sim code. We can convert them to forEach or mark them as eslint-disable-line.

[samreid]

Pushed new lint rule that prevents for...of. This will prevent it from leaking into our sims, and will help encourage us to use consistent style across sims + tools. I suspect some team members may prefer to use for...of in tools without having to specify // eslint-disable-line no-restricted-syntax, but it would seem unfortunate to divide our linting rules. May require further discussion.

[jonathanolson]

I suspect some team members may prefer to use for...of in tools without having to specify // eslint-disable-line no-restricted-syntax, but it would seem unfortunate to divide our linting rules.

Definitely don't want to have to provide an eslint disable for most of my for-loops in Node.js code.

How about we deliberately have separate Node.js and browser rules? For example, you never want to reference a global window in Node.js since it doesn't exist. But presumably we don't want to have an eslint line disable comment for every usage of window, etc. (or would you feel otherwise?)

[jessegreenberg]

Possibility to extend eslintrc files? https://eslint.org/docs/developer-guide/shareable-configs#using-a-shareable-config

[pixelzoom]

12/13/18 dev meeting: @zepumph will investigate splitting into server (Node) vs browser lint rules.

[zepumph]

I believe everything has been done for this issue. Further work in linked issues above.

Mighty inconvenient, but I guess we'll have to live with it.

@pixelzoom as for the error you pointed out, I have two thoughts.

1. The chipper lint config doesn't change all the time, but if we added a lint rule that the old rc of BCE failed, then it would be reasonable to think that you would have to checkout old chipper in order to get lint to pass. To me finding a bug because some shas are on a branch and others aren't comes with the territory, though I do admit that lint wasn't failing for those cases, and not is.
2. I think I have a potential solution, though it isn't ideal. The error is because we have moved to a tree structure in eslint configs, so if the linter looks at parent directories until it finds an eslint config file, for most repos, that is in the package.json. What you can do is add .eslintrc.js to the directory that contains all of the phet repos, so the parent to chipper and sims and whatnot, and have its contents look like:

module.exports = {
  extends: './chipper/eslint/.eslintrc.js',
};

Then the linter will fall back to this if no config is found in a given repo. I tested this by adding the file to my repo parent dir, and then checking out BCE 1.2. grunt lint is successful (though note it fails lint because of added lint rules in the past few weeks).

@pixelzoom let me know if this is something you are interested in pursuing as a workaround. Otherwise feel free to close.

[pixelzoom]

Thanks for investigating options.

I'll live with having to checkout-shas in order to cherry pick.

Closing.