Investigate ESLint

[samreid]

In #195 we determined that JSHint does not provide support new linting rules or tests. However, it seems like https://github.com/eslint/eslint is becoming very popular and it looks like it has backward support for JSHint rules and the ability to add your own rules.

If we are going to be adding more of our lint style rules like in #195 then we may want to invest in ESLint. No plans to look into it now but wanted to bring it up for future investigation.

[samreid]

@ariel-phet suggested to defer to an upcoming meeting for scheduling work on this.

[samreid]

ESLint will probably take longer to process than JSHint since it produces an AST for each file.

[samreid]

I'm interested to investigate this, but don't think I'll have time until after Bending Light is published.

@pixelzoom said this may not be worth the effort at the moment, may be more important if/when we re-engage with 3rd parties.

@samreid suggested investigating this when someone is reviewing a simulation and wishing for better automated support.

[samreid]

ESLint 1.4.0 was released recently, with support for cached results which will improve speed significantly (linting only occurs on files that have changed since last time of lint). Come to think of it, this may even be a speed improvement over our jshint process. So my concerns regarding performance are significantly allayed.

http://eslint.org/blog/2015/09/eslint-v1.4.0-released/

[samreid]

I've been enjoying the IDEA jshint plugin and noticed it has a similar ESLint plugin. It would be very nice to catch ESLint issues in the IDE while coding.

Thinking about how to approach ESLint--we wouldn't have to immediately bail on jshint and jump ship to ESLint, we could add an eslint phase to our existing process that only checks for specific things (that we miss with jshint).

I just saw another file where the constructor name didn't match the filename and it confused me for a bit. This seems like a natural thing to look for in ESLint--if there are any constructor functions, make sure at least one matches the filename. And I confirmed that ESLint context provides filename.

[samreid]

@jbphet @jonathanolson say ok to investigate ESLint as an add-on step as long as it doesn't slow down the build.

[samreid]

Assigned to myself (low priority) to take a look and see how this would impact our toolchain.

[samreid]

Testing for ESLint performance, using 3 eslint tasks in bending-light I see:

time grunt lint-all // 14.358s time grunt eslint-all // 14.634s (first time) time grunt eslint-all // 1.886s (second time, with cache)

After introducing an error into one file (3 missing ;): time grunt eslint-all // 1.928s time grunt lint-all // 14.072s

So a cached eslint with a small number of modified files takes about 14% of the time of running jshint. Without caching (or if all files changed) the times seem comparable.

A huge difference in the formatting:

vs

For me the eslint output is far easier to grok.

[samreid]

I'm elated at the idea of a linting step that takes 2 seconds instead of 14 seconds. Elated enough to abandon jshint completely and replace it with eslint? Perhaps.

[samreid]

Guess who wouldn't have to deal with grunt --lint=false anymore. This guy:

[samreid]

Checking for the cost (in time) of adding a cached eslint step to our existing process (default grunt, which includes jshint+requirejs+etc)

with eslint: 51.014s without eslint: 51.344s

So it looks like the time added by a cached eslint is in the noise.

[samreid]

For reference, here are the default eslinting rules our code doesn't follow:

/Users/samreid/github/axon/js/Property.js
  262:44  error  Unexpected console statement  no-console

/Users/samreid/github/axon/js/PropertySet.js
  72:22  error  Unexpected console statement  no-console

/Users/samreid/github/chipper/js/common/getDeployConfig.js
  50:32  error  "process" is not defined  no-undef

/Users/samreid/github/chipper/js/grunt/Gruntfile.js
  22:5   error  "_" is read only       no-undef
  92:43  error  Empty block statement  no-empty

/Users/samreid/github/chipper/js/grunt/checkoutShas.js
  38:28  error  Strings must use singlequote  quotes

/Users/samreid/github/chipper/js/grunt/createDependenciesJSON.js
  18:5  error  "_" is read only  no-undef

/Users/samreid/github/chipper/js/grunt/createSim.js
  82:63  error  Empty block statement  no-empty

/Users/samreid/github/chipper/js/grunt/getBuildConfig.js
   39:5   error  "_" is read only          no-undef
  254:43  error  "process" is not defined  no-undef

/Users/samreid/github/chipper/js/grunt/getGruntConfig.js
  11:5  error  "_" is read only  no-undef

/Users/samreid/github/chipper/js/grunt/getThirdPartyLibEntries.js
  16:5  error  "_" is read only  no-undef

/Users/samreid/github/chipper/js/grunt/reportMedia.js
  42:19  error  "process" is not defined  no-undef

/Users/samreid/github/chipper/js/grunt/reportThirdParty.js
  56:21  error  "process" is not defined  no-undef

/Users/samreid/github/chipper/js/grunt/reportUnusedMedia.js
  28:19  error  "process" is not defined  no-undef

/Users/samreid/github/chipper/js/requirejs-plugins/image.js
  46:22  error  Empty block statement  no-empty

/Users/samreid/github/chipper/js/requirejs-plugins/mipmap.js
  134:22  error  Empty block statement  no-empty

/Users/samreid/github/chipper/js/requirejs-plugins/registerLicenseEntry.js
  56:9  error  Unexpected console statement  no-console

/Users/samreid/github/chipper/js/requirejs-plugins/string.js
  193:19  error  Unexpected console statement  no-console
  193:32  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Complex.js
  77:14  error  Strings must use singlequote  quotes
  77:36  error  Strings must use singlequote  quotes
  77:52  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Dimension2.js
  25:14  error  Strings must use singlequote  quotes
  25:33  error  Strings must use singlequote  quotes
  25:55  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/LUDecomposition.js
  158:26  error  Strings must use singlequote  quotes
  170:26  error  Strings must use singlequote  quotes
  173:26  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Matrix.js
  323:28  error  Strings must use singlequote  quotes
  398:26  error  Strings must use singlequote  quotes
  403:20  error  Strings must use singlequote  quotes
  404:17  error  Strings must use singlequote  quotes
  404:52  error  Strings must use singlequote  quotes
  404:86  error  Strings must use singlequote  quotes
  407:44  error  Strings must use singlequote  quotes
  409:19  error  Strings must use singlequote  quotes
  486:24  error  Strings must use singlequote  quotes
  513:24  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Matrix3.js
  983:7  error  Unexpected console statement  no-console

/Users/samreid/github/dot/js/Matrix4.js
  527:27  error  Strings must use singlequote  quotes
  527:46  error  Strings must use singlequote  quotes
  527:65  error  Strings must use singlequote  quotes
  527:84  error  Strings must use singlequote  quotes
  528:27  error  Strings must use singlequote  quotes
  528:46  error  Strings must use singlequote  quotes
  528:65  error  Strings must use singlequote  quotes
  528:84  error  Strings must use singlequote  quotes
  529:27  error  Strings must use singlequote  quotes
  529:46  error  Strings must use singlequote  quotes
  529:65  error  Strings must use singlequote  quotes
  529:84  error  Strings must use singlequote  quotes
  530:27  error  Strings must use singlequote  quotes
  530:46  error  Strings must use singlequote  quotes
  530:65  error  Strings must use singlequote  quotes
  535:26  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Permutation.js
   83:28  error  Strings must use singlequote  quotes
   83:66  error  Strings must use singlequote  quotes
  124:14  error  Strings must use singlequote  quotes
  124:40  error  Strings must use singlequote  quotes
  124:49  error  Strings must use singlequote  quotes
  130:5   error  Unexpected console statement  no-console
  133:5   error  Unexpected console statement  no-console
  135:5   error  Unexpected console statement  no-console
  137:5   error  Unexpected console statement  no-console

/Users/samreid/github/dot/js/QRDecomposition.js
  139:26  error  Strings must use singlequote  quotes
  142:26  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Range.js
  68:14   error  Strings must use singlequote  quotes
  68:42   error  Strings must use singlequote  quotes
  68:63   error  Strings must use singlequote  quotes
  68:102  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Ray2.js
  36:41  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Ray3.js
  37:36  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Vector2.js
  119:26  error  Strings must use singlequote  quotes
  289:26  error  Strings must use singlequote  quotes
  334:24  error  Strings must use singlequote  quotes
  334:79  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Vector3.js
   98:26  error  Strings must use singlequote  quotes
  162:14  error  Strings must use singlequote  quotes
  162:36  error  Strings must use singlequote  quotes
  162:52  error  Strings must use singlequote  quotes
  162:68  error  Strings must use singlequote  quotes
  248:26  error  Strings must use singlequote  quotes
  285:24  error  Strings must use singlequote  quotes
  285:79  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Vector4.js
   88:26  error  Strings must use singlequote  quotes
  148:14  error  Strings must use singlequote  quotes
  148:36  error  Strings must use singlequote  quotes
  148:52  error  Strings must use singlequote  quotes
  148:68  error  Strings must use singlequote  quotes
  148:84  error  Strings must use singlequote  quotes
  229:26  error  Strings must use singlequote  quotes
  255:24  error  Strings must use singlequote  quotes
  255:79  error  Strings must use singlequote  quotes

/Users/samreid/github/joist/js/PhetMenu.js
  239:11  error  Unexpected console statement  no-console

/Users/samreid/github/joist/js/Profiler.js
  58:25  error  Strings must use singlequote  quotes

/Users/samreid/github/joist/js/Sim.js
  466:24  error  Strings must use singlequote                            quotes
  636:13  error  Redundant double negation in an if statement condition  no-extra-boolean-cast

/Users/samreid/github/joist/js/SimJSON.js
  30:18  error  Strings must use singlequote  quotes

/Users/samreid/github/joist/js/UpdateCheck.js
  121:15  error  Unexpected console statement  no-console
  134:17  error  Unexpected console statement  no-console

/Users/samreid/github/phet-core/js/loadScript.js
  42:31  error  Strings must use singlequote  quotes
  42:55  error  Strings must use singlequote  quotes

/Users/samreid/github/phet-core/js/phetAllocation.js
  44:11  error  Unexpected console statement  no-console

/Users/samreid/github/phetcommon/js/model/SphereBucket.js
  63:7  error  Unexpected console statement  no-console

/Users/samreid/github/scenery/js/debug/DebugContext.js
  28:5  error  Unexpected console statement  no-console

/Users/samreid/github/scenery/js/display/GreedyStitcher.js
  124:7  error  Unexpected constant condition  no-constant-condition
  325:9  error  Unexpected constant condition  no-constant-condition

/Users/samreid/github/scenery/js/display/PixiBlock.js
  111:7  error  Unexpected console statement  no-console

/Users/samreid/github/scenery/js/display/webgl/ColorTriangleBufferData.js
  86:9  error  Unexpected console statement  no-console

/Users/samreid/github/scenery/js/input/Input.js
  360:9   error  Unexpected constant condition  no-constant-condition
  399:9   error  Unexpected constant condition  no-constant-condition
  407:9   error  Unexpected constant condition  no-constant-condition
  575:18  error  Unexpected console statement   no-console
  576:15  error  Unexpected console statement   no-console
  593:18  error  Unexpected console statement   no-console
  594:15  error  Unexpected console statement   no-console
  602:29  error  Unexpected console statement   no-console
  603:15  error  Unexpected console statement   no-console
  613:18  error  Unexpected console statement   no-console
  614:15  error  Unexpected console statement   no-console
  631:18  error  Unexpected console statement   no-console
  632:15  error  Unexpected console statement   no-console
  775:13  error  Unexpected console statement   no-console

/Users/samreid/github/scenery/js/nodes/Image.js
  69:30  error  Strings must use singlequote  quotes

/Users/samreid/github/scenery/js/nodes/Node.js
   854:42  error  Empty block statement         no-empty
  2416:33  error  Strings must use singlequote  quotes
  3487:22  error  Empty block statement         no-empty

/Users/samreid/github/scenery/js/nodes/Text.js
  903:68  error  Empty block statement  no-empty

/Users/samreid/github/scenery/js/util/Color.js
  475:38   error  Strings must use singlequote  quotes
  475:55   error  Strings must use singlequote  quotes
  475:72   error  Strings must use singlequote  quotes
  475:89   error  Strings must use singlequote  quotes
  475:106  error  Strings must use singlequote  quotes

/Users/samreid/github/scenery/js/util/ShaderProgram.js
  50:9  error  Unexpected console statement  no-console
  51:9  error  Unexpected console statement  no-console
  52:9  error  Unexpected console statement  no-console
  53:9  error  Unexpected console statement  no-console
  54:9  error  Unexpected console statement  no-console
  55:9  error  Unexpected console statement  no-console

/Users/samreid/github/scenery/js/util/Util.js
  286:13  error  Unexpected console statement  no-console
  287:13  error  Unexpected console statement  no-console
  288:13  error  Unexpected console statement  no-console
  309:13  error  Unexpected console statement  no-console
  330:13  error  Unexpected console statement  no-console
  351:13  error  Unexpected console statement  no-console
  363:9   error  Unexpected console statement  no-console
  364:9   error  Unexpected console statement  no-console
  408:9   error  Unexpected console statement  no-console
  409:9   error  Unexpected console statement  no-console
  410:9   error  Unexpected console statement  no-console

/Users/samreid/github/scenery-phet/js/demo/ButtonsView.js
  33:30  error  Unexpected console statement  no-console
  47:30  error  Unexpected console statement  no-console

/Users/samreid/github/scenery-phet/js/demo/ComponentsView.js
  139:7  error  Unexpected console statement  no-console

/Users/samreid/github/scenery-phet/js/demo/SpringControls.js
  27:21  error  Strings must use singlequote  quotes

/Users/samreid/github/sun/js/Carousel.js
  98:45  error  Unexpected trailing comma  comma-dangle

/Users/samreid/github/sun/js/FontAwesomeNode.js
  18:17  error  Strings must use singlequote  quotes
  19:14  error  Strings must use singlequote  quotes
  20:11  error  Strings must use singlequote  quotes
  21:17  error  Strings must use singlequote  quotes
  22:16  error  Strings must use singlequote  quotes
  23:12  error  Strings must use singlequote  quotes
  24:18  error  Strings must use singlequote  quotes
  25:11  error  Strings must use singlequote  quotes
  26:10  error  Strings must use singlequote  quotes
  27:12  error  Strings must use singlequote  quotes
  28:17  error  Strings must use singlequote  quotes
  29:19  error  Strings must use singlequote  quotes
  30:15  error  Strings must use singlequote  quotes
  31:16  error  Strings must use singlequote  quotes
  32:19  error  Strings must use singlequote  quotes
  45:13  error  Strings must use singlequote  quotes

✖ 172 problems (172 errors, 0 warnings)

[samreid]

I don't want to spend much time on this without getting a +1 from other devs, but I do want to commit my changes so that we can easily continue if we decide to.

[samreid]

I posted this in the google group:

https://groups.google.com/forum/#!forum/developing-interactive-simulations-in-html5

I'm investigating ESLint support in chipper as part of https://github.com/phetsims/chipper/issues/235 . The package.json got 2 new dependencies, so developers and build processes will likely have to run npm install from chipper before the next build.

Please report any issues here, and have a nice weekend! Sam

[samreid]

To get my eslint rule sealegs, I'm writing a rule that checks that each file has a copyright statement. Here are some typos/errors I'm seeing (and fixing):

// Copyright 2002-2014, University of Colorado Boulder Boulder
// Copyright 2002-2014, University of Colorado
/*
 * Copyright 2002-2014, University of Colorado Boulder
 */

etc.

[samreid]

Writing my own rule and applying it was a very positive experience. However, I'm unable to get IDEA to use my dynamic rules in its checking. It shows this error:

[samreid]

Moving from "additional rules directory" to the options seems to have fixed this: --rulesdir /Users/samreid/github/chipper/eslint/rules

[samreid]

Our style guide directs us to use single quotes.

However, eslint reports 100 occurrences of double quotes (when doing lint-all from bending-light).

~/github/bending-light$ grunt eslint
Running "eslint:target" (eslint) task

/Users/samreid/github/chipper/js/grunt/checkoutShas.js
  38:28  error  Strings must use singlequote  quotes

/Users/samreid/github/chipper/js/requirejs-plugins/string.js
  193:32  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Complex.js
  77:14  error  Strings must use singlequote  quotes
  77:36  error  Strings must use singlequote  quotes
  77:52  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Dimension2.js
  25:14  error  Strings must use singlequote  quotes
  25:33  error  Strings must use singlequote  quotes
  25:55  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/LUDecomposition.js
  158:26  error  Strings must use singlequote  quotes
  170:26  error  Strings must use singlequote  quotes
  173:26  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Matrix.js
  323:28  error  Strings must use singlequote  quotes
  398:26  error  Strings must use singlequote  quotes
  403:20  error  Strings must use singlequote  quotes
  404:17  error  Strings must use singlequote  quotes
  404:52  error  Strings must use singlequote  quotes
  404:86  error  Strings must use singlequote  quotes
  407:44  error  Strings must use singlequote  quotes
  409:19  error  Strings must use singlequote  quotes
  486:24  error  Strings must use singlequote  quotes
  513:24  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Matrix4.js
  527:27  error  Strings must use singlequote  quotes
  527:46  error  Strings must use singlequote  quotes
  527:65  error  Strings must use singlequote  quotes
  527:84  error  Strings must use singlequote  quotes
  528:27  error  Strings must use singlequote  quotes
  528:46  error  Strings must use singlequote  quotes
  528:65  error  Strings must use singlequote  quotes
  528:84  error  Strings must use singlequote  quotes
  529:27  error  Strings must use singlequote  quotes
  529:46  error  Strings must use singlequote  quotes
  529:65  error  Strings must use singlequote  quotes
  529:84  error  Strings must use singlequote  quotes
  530:27  error  Strings must use singlequote  quotes
  530:46  error  Strings must use singlequote  quotes
  530:65  error  Strings must use singlequote  quotes
  535:26  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Permutation.js
   83:28  error  Strings must use singlequote  quotes
   83:66  error  Strings must use singlequote  quotes
  124:14  error  Strings must use singlequote  quotes
  124:40  error  Strings must use singlequote  quotes
  124:49  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/QRDecomposition.js
  139:26  error  Strings must use singlequote  quotes
  142:26  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Range.js
  66:14   error  Strings must use singlequote  quotes
  66:42   error  Strings must use singlequote  quotes
  66:63   error  Strings must use singlequote  quotes
  66:102  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Ray2.js
  36:41  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Ray3.js
  37:36  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Vector2.js
  119:26  error  Strings must use singlequote  quotes
  289:26  error  Strings must use singlequote  quotes
  334:24  error  Strings must use singlequote  quotes
  334:79  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Vector3.js
   98:26  error  Strings must use singlequote  quotes
  162:14  error  Strings must use singlequote  quotes
  162:36  error  Strings must use singlequote  quotes
  162:52  error  Strings must use singlequote  quotes
  162:68  error  Strings must use singlequote  quotes
  248:26  error  Strings must use singlequote  quotes
  285:24  error  Strings must use singlequote  quotes
  285:79  error  Strings must use singlequote  quotes

/Users/samreid/github/dot/js/Vector4.js
   88:26  error  Strings must use singlequote  quotes
  148:14  error  Strings must use singlequote  quotes
  148:36  error  Strings must use singlequote  quotes
  148:52  error  Strings must use singlequote  quotes
  148:68  error  Strings must use singlequote  quotes
  148:84  error  Strings must use singlequote  quotes
  229:26  error  Strings must use singlequote  quotes
  255:24  error  Strings must use singlequote  quotes
  255:79  error  Strings must use singlequote  quotes

/Users/samreid/github/joist/js/Profiler.js
  58:25  error  Strings must use singlequote  quotes

/Users/samreid/github/joist/js/Sim.js
  466:24  error  Strings must use singlequote  quotes

/Users/samreid/github/joist/js/SimJSON.js
  30:18  error  Strings must use singlequote  quotes

/Users/samreid/github/phet-core/js/loadScript.js
  42:31  error  Strings must use singlequote  quotes
  42:55  error  Strings must use singlequote  quotes

/Users/samreid/github/scenery/js/nodes/Image.js
  69:30  error  Strings must use singlequote  quotes

/Users/samreid/github/scenery/js/nodes/Node.js
  2416:33  error  Strings must use singlequote  quotes

/Users/samreid/github/scenery/js/util/Color.js
  475:38   error  Strings must use singlequote  quotes
  475:55   error  Strings must use singlequote  quotes
  475:72   error  Strings must use singlequote  quotes
  475:89   error  Strings must use singlequote  quotes
  475:106  error  Strings must use singlequote  quotes

/Users/samreid/github/scenery-phet/js/demo/SpringControls.js
  27:21  error  Strings must use singlequote  quotes

/Users/samreid/github/sun/js/FontAwesomeNode.js
  18:17  error  Strings must use singlequote  quotes
  19:14  error  Strings must use singlequote  quotes
  20:11  error  Strings must use singlequote  quotes
  21:17  error  Strings must use singlequote  quotes
  22:16  error  Strings must use singlequote  quotes
  23:12  error  Strings must use singlequote  quotes
  24:18  error  Strings must use singlequote  quotes
  25:11  error  Strings must use singlequote  quotes
  26:10  error  Strings must use singlequote  quotes
  27:12  error  Strings must use singlequote  quotes
  28:17  error  Strings must use singlequote  quotes
  29:19  error  Strings must use singlequote  quotes
  30:15  error  Strings must use singlequote  quotes
  31:16  error  Strings must use singlequote  quotes
  32:19  error  Strings must use singlequote  quotes
  45:13  error  Strings must use singlequote  quotes

✖ 100 problems (100 errors, 0 warnings)

I spot checked these errors and they look legit. The quotes rule reportedly has a fix option, so I'm going to give it a go.

[samreid]

I used eslint's autofix to automatically fix all of the incorrect quotes. I spot checked and it looked like all of the fixes were good. I launched Bending Light and it seemed to run properly. So I committed (above).

[samreid]

After fixes above, it seems like we are satisfying almost all of the recommended linting rules with eslint: http://eslint.org/docs/rules/

The rules we aren't satisfying are:

• Empty block statements
• Redundant double negation in an if statement condition
• Unexpected constant condition

@jonathanolson some of these are scenery issues, can you take a look and see if you want to go with eslint's rules or turn them off.

[samreid]

require-jsdoc reports 155 functions with missing jsdoc. We may want to add that documentation!

[samreid]

To take eslint for a test drive, you must do the following: pull-all.sh cd chipper npm install

then add this line to your simulation package.json/devDependencies: "grunt-eslint": "~17.2.0",

then do these commands cd my-simulation grunt eslint

The eslint task will lint the sim and its dependencies. In the future eslint should become part of the standard grunt.

[samreid]

I'm going to take a break from this issue and wait for feedback from other developers. If we decide to proceed with eslint, we will need better documentation in .eslintrc and to decide on the set of rules we need.

At this point in time, my recommendation is to abandon jshint and make eslint a part of the standard build. But I feel like we need consensus before this kind of change. So I'll tag @aaronsamuel137 @jbphet @jonathanolson @pixelzoom @jessegreenberg for feedback and mark this for developer-meeting. It would also be ok (though not necessary) If we can get it sorted out asynchronously before the next developer meeting. Also note this is not high priority and not critical to get done quickly.

[jonathanolson]

The increase in time sounds amazing - does this mean if you run grunt from one repo, it speeds it up in another? Or just linting one repo only speeds up that repo in the future? Either way, excited for that.

Unexpected constant condition

For "Unexpected constant condition", one usage is basically:

var value = ...;
while ( true ) {
  var nextValue = value;
  if ( isNextValueAcceptable( nextValue ) ) {
    value = nextValue;
  }
  else {
    break;
  }
}

whereas putting the condition check in the while loop causes duplication (performance loss) of the call to getNext:

var value = ...;
while ( isNextValueAcceptable( getNext( value ) ) ) {
  value = getNext( value )
}

or would require using the result of an assignment expression:

var value = ...;
var nextValue;
while ( isNextValueAcceptable( nextValue = getNext( value ) ) ) {
  value = nextValue;
}

Any thoughts on the preferred option that would lint, and ideally not call getNext twice?

Empty block statement:

Also another example is the empty block statement:

while ( doSideEffectsUntilReturnsTrue() ) {}

which could be turned into the following EXCEPT it would run afoul of the other linter rule:

while ( true ) {
  if ( doSideEffectsUntilReturnsTrue() ) {
    break;
  }
}

or the equivalent:

for ( ;; ) {
  if ( doSideEffectsUntilReturnsTrue() ) {
    break;
  }
}

or using recursion might lead into call stack limits:

( function doIt() {
  if ( !doSideEffectsUntilReturnsTrue() ) {
    doIt();
  }
} )();

or other ways of getting around the dislike of empty blocks that are annoying:

while ( doSideEffectsUntilReturnsTrue() ) {
  assert && assert ( true, 'I will do nothing' );
}

or possibly refactoring that out:

var loopUntilTrue = require( 'PHET_CORE/loopUntilTrue' );

loopUntilTrue( doSideEffectsUntilReturnsTrue );

Another empty block statement

The empty block check also runs afoul of another place:

try {
  somethingThatMayThrowThingsButThatIsOk();
}
catch ( e ) {}

which could be worked around with the same "phony assert".

Summary

So overall it seems like a great win (and other lint issues I didn't mention in my code look like things that should be fixed), and I'd be excited to have it. Curious about how to handle the above patterns in a nice way.

[samreid]

does this mean if you run grunt from one repo, it speeds it up in another?

The default is to create a cache file in each repo, but there is the option to specify a cache file, in which case I think results might be shareable. We would have to test it.

It looks like a for loop is a possible replacement for while(true):

  for (value='a';isNextValueAcceptable(value);value=getNextValue(value)){

  }

I will make a note to look into the other issues in the future.

[pixelzoom]

Does IDEA have any support for ESLint, along the lines of what was being pursued in https://github.com/phetsims/chipper/issues/372?

[aaronsamuel137]

Took it for a test drive in GAO. Found a few issues not caught by jshint, such as double quotes and incorrect copyright statements. Seems useful to me. And the speedup is great!

+1 for moving forward with it.

[samreid]

Does IDEA have any support for ESLint, along the lines of what was being pursued in #372?

Yes, support seems basically equivalent and it does support custom rules. I had to specify the custom rules directory like described in this comment https://github.com/phetsims/chipper/issues/235#issuecomment-145161202

Here's a picture of IDEA highlighting a bogus copyright statement (with a custom rule I wrote):

[pixelzoom]

"Empty block" errors can also be suppressed by putting a comment in the block, something like this:

if ( expression ) {
  // do nothing
}
else {
   statement1;
   ...
}

Similar for try/catch.

[pixelzoom]

I took ESLint for a test drive. +1 for moving forward with it, if we're willing to spend the time to do a grunt-all.sh eslint and clean everything up.

[jonathanolson]

It looks like a for loop is a possible replacement for while(true):

  for (value='a';isNextValueAcceptable(value);value=getNextValue(value)){

  }

Wouldn't the pattern be:

for ( value = 'a'; isNextValueAcceptable( getNextValue( value ) ); value = getNextValue( value ) ) {
  // comment to suppress warning
}

since getNextValue() needs to be called on the value before the isNextValueAcceptable check? This duplicates the getNextValue() call. Alternatively:

for ( var value = 'a', nextValue = null;
      nextValue = getNextValue( value ), isNextValueAcceptable( nextValue );
      value = getNextValue( value ) ) {
  // comment to suppress warning
}

but that looks way more complicated to me.

[samreid]

Wouldn't the pattern be [...]

Yes, it looks like you are correct. One option would be to use code comment directives to turn off eslint features where we know we want to go outside one of the rules.

It may also work to put: var t=true; while(t){}...

I'm not sure how important the Unexpected constant condition rule truly is though.

[samreid]

if we're willing to spend the time to do a grunt-all.sh eslint and clean everything up.

Yes, that sounds like a good condition.

When @pixelzoom and @aaronsamuel137 said ok to move forward with it, does that mean ok to delete all of our jshint support?

[pixelzoom]

does that mean ok to delete all of our jshint support?

Might want to run "grunt-all.sh eslint" and clean everything up before deleting jshint support, in case anything major is encountered during that exercise.

[samreid]

@samreid should add .eslintcache to the .gitignore

[samreid]

It looks like the eslint defaults are enforcing LF rather than CR or CRLF. I'm only seeing a few sims not using this standard (such as sugar and salt solutions), so I'll update them.

[samreid]

I sent @phetsims/ashrafsharf this email:

Hi Ashraf,

We are switching from jshint to eslint and one of the rules we want to enforce is that the line endings are LF instead of CRLF. I fixed them in sugar and salt solutions and will fix them in gene expression basics next, but wanted to notify you about this change. Can you work with your team to make sure it is doing LF in the future?

See https://github.com/phetsims/chipper/issues/235#issuecomment-145975044 for context.

Best Regards, Sam

[pixelzoom]

@samreid Question about ESLint caching... If the options to ESLint are changed, is it smart enough to invalidate the caches?

[samreid]

@samreid Question about ESLint caching... If the options to ESLint are changed, is it smart enough to invalidate the caches?

Changing the rules in .eslintrc => caches invalidated Changing the command-line style options in gruntConfig => caches not invalidated

[samreid]

isotopes and atomic mass is giving this odd message:

~/github/isotopes-and-atomic-mass$ grunt eslint
Running "eslint:target" (eslint) task
Warning: Cannot read property 'value' of undefined Use --force to continue.

Aborted due to warnings.

[pixelzoom]

Looks like "grunt-eslint": "~17.2.0" was added to package.json for all sims, but not for other (common-code) repos. Just a reminder that they need to be eslintable too.

[pixelzoom]

Looks like some common-code repos do have "grunt-eslint": "~17.2.0" (e.g. axon) while others don't (e.g. scenery-phet, sun).

[samreid]

After the above commits grunt-all.sh eslint is now passing.

[pixelzoom]

% cd sun
% grunt eslint
Warning: Task "eslint" not found. Use --force to continue.

Aborted due to warnings.
%

[pixelzoom]

I had apparently picked up a transient version of chipper that had "auto-fix changes" turned on for ESLint, and it had changed a bunch of files, so my pull-all.sh was failing. Revert changes, pulled latest chipper, and everything looks good.

[samreid]

I changed grunt-all to use active-repos instead of active-runnables and went through a build to make sure all appropriate repos have eslint, and are linting properly. Optics lab is still an exception (as it was for jshint), but unless I have made a manual error, everything else is good to go.

Next steps:

1. Integrate eslint into the main build process
2. Get rid of jshint
3. add .gitignore for the .eslint cache files
4. check through to see if there are other prebuilt rules we want/need
5. add documentation to the eslint files in chipper

This "Investigate ESList" thread is getting a bit long and indeed our "investigation" is over, so I'll move the remaining work to a new issue and close this one.

[samreid]

New issue created above, I'll close this one.

[samreid]

One more answer to this question:

Question about ESLint caching... If the options to ESLint are changed, is it smart enough to invalidate the caches?

It appears if you change the .js code of a rule definition, it does *not invalidate caches.

[pixelzoom]

We could add support for deleting caches to the build process. But you can delete all caches like this:

% cd /Users/cmalley/PhET/GitHub
% rm */.eslintcache

[samreid]

I also added a grunt command line argument --disable-eslint-cache which you can be used for iterating on rules.