Update to ESLint 7

[samreid]

As described in phetsims/phet-info#150 (comment)

Upgrade to ESLint 7 (we are currently on 6, which is missing some style rules).

The migration guide is here: https://eslint.org/docs/user-guide/migrating-to-7.0.0

[samreid]

I noticed weddell is already using eslint 7.

[samreid]

ESLint 7 has a new default rule that prevents setters from returning a value. I considered turning the rule off for compatibility, but it seems better to eliminate these returns, so I'll commit that.

[samreid]

Following the migration guide linked above, I adapted our grunt lint to use ESLint 7. The most significant changes were:

• Use new ESLint instead of CLIEngine
• lintFiles is now async, so I converted the whole function to async
• I added a command line parameter --fix which will run the ESLint fixes

I ran the following tests on the new version:

• Create a lint error in fourier and grunt lint in fourier.
• Create a lint error in fourier and grunt lint-everything from perennial.
• Test caching for lint-everything (see below)
• Test caching for one repo (see below)
• Make sure cache files appear where desired
• Make sure our custom rules are respected
• Make sure the .eslintignore file is respected. I tried disrupting one of each kind of path and saw that it had the desired behavior.
• Test precommit hooks. There was a problem that process.exit was called too soon in the puppeteer part. I had to eliminate that to give the lint time to run. I also labeled the function with a version string so that we can choose to only run the linting with compatible SHAs.

~/apache-document-root/main/perennial$ time grunt lint-everything --disable-eslint-cache
Running "lint-everything" task
(node:29823) Warning: Accessing non-existent property 'padLevels' of module exports inside circular dependency
(Use `node --trace-warnings ...` to show where the warning was created)

Done.

real	0m50.599s
user	1m0.679s
sys	0m3.901s
~/apache-document-root/main/perennial$ time grunt lint-everything
Running "lint-everything" task
(node:29835) Warning: Accessing non-existent property 'padLevels' of module exports inside circular dependency
(Use `node --trace-warnings ...` to show where the warning was created)

Done.

real	0m52.462s
user	1m2.254s
sys	0m3.950s
~/apache-document-root/main/perennial$ time grunt lint-everything
Running "lint-everything" task
(node:29838) Warning: Accessing non-existent property 'padLevels' of module exports inside circular dependency
(Use `node --trace-warnings ...` to show where the warning was created)

Done.

real	0m3.875s
user	0m2.793s
sys	0m0.893s
~/apache-document-root/main/perennial$ 

############

~/apache-document-root/main/fourier-making-waves$ time grunt lint --disable-eslint-cache
Running "lint" task

Done.

real	0m4.200s
user	0m3.016s
sys	0m0.713s
~/apache-document-root/main/fourier-making-waves$ time grunt lint
Running "lint" task

Done.

real	0m2.097s
user	0m2.437s
sys	0m0.358s
~/apache-document-root/main/fourier-making-waves$ 
~/apache-document-root/main/fourier-making-waves$ time grunt lint
Running "lint" task

Done.

real	0m1.498s
user	0m1.443s
sys	0m0.312s

~/apache-document-root/main/fourier-making-waves$ time grunt lint --disable-eslint-cache
Running "lint" task

Done.

real	0m3.812s
user	0m2.896s
sys	0m0.592s
~/apache-document-root/main/fourier-making-waves$ time grunt lint
Running "lint" task

Done.

real	0m2.633s
user	0m2.564s
sys	0m0.409s
~/apache-document-root/main/fourier-making-waves$ time grunt lint
Running "lint" task

Done.

real	0m1.468s
user	0m1.410s
sys	0m0.309s

Everything worked as desired, so I'll take the following steps:

• Commit to master
• Notify slack, since a npm update will be necessary in chipper
• Notify the google group for the same reason
• Assign to @ariel-phet to request review. The other developers with commits on lint.js are primarily: @jonathanolson @zepumph

[samreid]

While I was working on this, ESLint 7.17.0 was released, with an autofix for formatting ternary operators. I'm inclined to update to this now so we only need to notify clients and PhET developers to npm update once.

[samreid]

I posted a message in slack and in the google group:

Chipper has updated from ESLint 6 to 7, details are in #1000. Please npm update in chipper when you pull changes.

The last step is:

Assign to @ariel-phet to request review. The other developers with commits on lint.js are primarily: @jonathanolson @zepumph

[samreid]

CT discovered an issue when there are no files to lint (in phet-io-wrapper-classroom-activity), I corrected it in the commit.

[samreid]

On second thought, it makes sense for me to add a --files option before review, so I'll reassign to me until that's done.

[samreid]

I added support for --patterns=... in #1001 and I think that should be included in the review. Summarizing what should be reviewed:

• lint.js, including its options and async parts and chipperAPIVersion
• usages of lint including dynamically loaded parts, like in perennial, in the precommit hook, etc.

Questions:

• I noticed that perennial gruntfile has 2 ways of invoking lint, one is via execute and one is via await. Should both use same strategy? If so, which strategy?

[ariel-phet]

Assigning to @jonathanolson, since we use this tool quite a bit, I am recommending high priority.

[jonathanolson]

I'm trying to understand the let failed = true and other logic in hook-pre-commit.js seems

results.forEach( result => {
  let failed = false;
  if ( result.errorCount > 0 || result.warningCount > 0 ) {
    console.error( `lint failed in ${repo}`, result.filePath, result );
    failed = true;
  }
  if ( failed ) {
    process.exit( 1 );
  }
} );

could be simplified to:

for ( const result of results ) {
  if ( result.errorCount > 0 || result.warningCount > 0 ) {
    console.error( `lint failed in ${repo}`, result.filePath, result );
    process.exit( 1 );
  }
}

Also: preferring for-of loops in async functions, so that it will work nicely if we add awaits INSIDE the loop.

[jonathanolson]

  const sum = ( a, b ) => a + b;
  const count = numbers => numbers.length === 0 ? 0 : numbers.reduce( sum );

I believe can be reduced to:

  const count = numbers.reduce( ( a, b ) => a + b, 0 );

or, since we have lodash:

const count = _.sum( numbers );

[jonathanolson]

The comment here seems incorrect. I presume it's more "no JS code that we WANT to be linted exists here"?

const EXCLUDE_PATTERNS = [ // patterns that have no code and lint should not be attempted

[jonathanolson]

Looks good to me! Can you take a look at the potential improvements above?

[samreid]

I'm trying to understand the let failed = true and other logic in hook-pre-commit.js seems

The intent was that it would report all failing repos instead of just the first one.

[jonathanolson]

Sounds good, then the failed and if ( failed ) should be moved out, since it looks like it would process-exit on the first failure immediately.

[samreid]

Great recommendations, I addressed each above (and improved output formatting), ready for re-review.

[jonathanolson]

Looks great, thanks!