require-statement-match rule is broken

[zepumph]

Discussed during discussion in #1060.

[zepumph]

From @samreid in #1060 (comment):

perhaps use import-js/eslint-plugin-import#325

[zepumph]

I looked through that issue, and didn't see that anything has been implemented into the main ruleset yet. I did see https://github.com/minseoksuh/eslint-plugin-consistent-default-export-name, but it doesn't seem very used, popular, though seems a bit maintained.

I applied this patch, npm updated, and ran grunt lint in friction and found:

 Michael ~/PhET/git/friction (master)
 $ grunt lint
Running "lint" task

C:\Users\Michael\PhET\git\friction\js\friction\view\FrictionScreenView.js
   1:1  error  Resolve error: unable to load resolver "node"                                            consistent-default-export-name/default-import-match-filename
  21:8  error  Default import name does not match filename "break-off-autosinfonie-spatialized_mp3.js"  consistent-default-export-name/default-import-match-filename
  22:8  error  Default import name does not match filename "contact-lower_mp3.js"                       consistent-default-export-name/default-import-match-filename

C:\Users\Michael\PhET\git\friction\js\friction\view\MoleculeMotionSoundGenerator.js
   1:1  error  Resolve error: unable to load resolver "node"                        consistent-default-export-name/default-import-match-filename
  22:8  error  Default import name does not match filename "bounce-marimba_mp3.js"  consistent-default-export-name/default-import-match-filename

C:\Users\Michael\PhET\git\friction\js\friction\view\book\BookNode.js
   1:1  error  Resolve error: unable to load resolver "node"                       consistent-default-export-name/default-import-match-filename
  19:8  error  Default import name does not match filename "simple-drop_mp3.js"    consistent-default-export-name/default-import-match-filename
  20:8  error  Default import name does not match filename "simple-pickup_mp3.js"  consistent-default-export-name/default-import-match-filename

C:\Users\Michael\PhET\git\friction\js\friction\view\magnifier\MagnifierNode.js
   1:1  error  Resolve error: unable to load resolver "node"                     consistent-default-export-name/default-import-match-filename
  26:8  error  Default import name does not match filename "harp-drop_mp3.js"    consistent-default-export-name/default-import-match-filename
  27:8  error  Default import name does not match filename "harp-pickup_mp3.js"  consistent-default-export-name/default-import-match-filename

✖ 11 problems (11 errors, 0 warnings)

Warning: 11 errors and 0 warnings Use --force to continue.

Aborted due to warnings.

Index: eslint/.eslintrc.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/eslint/.eslintrc.js b/eslint/.eslintrc.js
--- a/eslint/.eslintrc.js	(revision f2e651ea5830b168f4b8c9ee5ad4fde0916ab853)
+++ b/eslint/.eslintrc.js	(date 1630079184011)
@@ -13,6 +13,8 @@
   // in a parent dir
   root: true,
 
+  extends: [ "plugin:consistent-default-export-name/fixed" ],
+
   // The rules are organized like they are in the list at https://eslint.org/docs/rules/
   // First by type, then alphabetically within type
   // Explicitly list all rules so it is easy to see what's here and to keep organized
Index: package.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/package.json b/package.json
--- a/package.json	(revision f2e651ea5830b168f4b8c9ee5ad4fde0916ab853)
+++ b/package.json	(date 1630079038788)
@@ -33,7 +33,8 @@
     "terser": "~4.6.4",
     "webpack": "^5.47.1",
     "webpack-cli": "^4.7.0",
-    "webpack-dev-server": "^3.11.2"
+    "webpack-dev-server": "^3.11.2",
+    "eslint-plugin-consistent-default-export-name": "^0.0.6"
   },
   "eslintConfig": {
     "extends": "../chipper/eslint/sim_eslintrc.js",

[samreid]

That looks promising, but if we are concerned about the resolve error, or whether it may be maintained, writing our own rule may not be too complicated, and would give us flexibility if we want to adjust or tinker with it. Not advocating strongly one way or the other, just brainstorming options.

[zepumph]

@jessegreenberg and @samreid, this feels like a step in the right direction. I'm unsure what the "Resolve error: . . ." is, but perhaps it doesn't matter. When we fix the actual error, we solve the node resolve error.

Index: js/friction/view/MoleculeMotionSoundGenerator.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/friction/view/MoleculeMotionSoundGenerator.js b/js/friction/view/MoleculeMotionSoundGenerator.js
--- a/js/friction/view/MoleculeMotionSoundGenerator.js	(revision cf117e5f215eb53100ce1d8da8f0c3ea0b892ea5)
+++ b/js/friction/view/MoleculeMotionSoundGenerator.js	(date 1630079957023)
@@ -19,7 +19,7 @@
 import merge from '../../../../phet-core/js/merge.js';
 import SoundClip from '../../../../tambo/js/sound-generators/SoundClip.js';
 import SoundGenerator from '../../../../tambo/js/sound-generators/SoundGenerator.js';
-import bounceMarimbaSound from '../../../sounds/bounce-marimba_mp3.js';
+import bounceMarimba_mp3 from '../../../sounds/bounce-marimba_mp3.js'; // eslint-disable-line import-resource-variable-suffix
 import friction from '../../friction.js';
 import FrictionConstants from '../FrictionConstants.js';
 import FrictionModel from '../model/FrictionModel.js';
@@ -41,10 +41,10 @@
 
     // create several instances of the sound clip at different volume levels to allow more variation in the sound
     const motionSoundClips = [
-      new SoundClip( bounceMarimbaSound, { initialOutputLevel: 1, rateChangesAffectPlayingSounds: false } ),
-      new SoundClip( bounceMarimbaSound, { initialOutputLevel: 0.75, rateChangesAffectPlayingSounds: false } ),
-      new SoundClip( bounceMarimbaSound, { initialOutputLevel: 0.5, rateChangesAffectPlayingSounds: false } ),
-      new SoundClip( bounceMarimbaSound, { initialOutputLevel: 0.25, rateChangesAffectPlayingSounds: false } )
+      new SoundClip( bounceMarimba_mp3, { initialOutputLevel: 1, rateChangesAffectPlayingSounds: false } ),
+      new SoundClip( bounceMarimba_mp3, { initialOutputLevel: 0.75, rateChangesAffectPlayingSounds: false } ),
+      new SoundClip( bounceMarimba_mp3, { initialOutputLevel: 0.5, rateChangesAffectPlayingSounds: false } ),
+      new SoundClip( bounceMarimba_mp3, { initialOutputLevel: 0.25, rateChangesAffectPlayingSounds: false } )
     ];
 
     // connect up the sound clips

Next I'll take a brief look at their rule to see if there is an easy or obvious pull request we could do to get rid of that error.

[zepumph]

I had good luck finding the small bug that was causing that weird error in the package. I created an issue pull request for the fix over in minseoksuh/eslint-plugin-consistent-default-export-name#7. Given the success I had with bug-fixing their project, I think we should go all in on this package. I'm going to add this dependency, and add it to the eslintrc file turned off (since there will be many, many errors). I'll report back soon to see how it goes.

[zepumph]

I ran grunt lint-everything, and found 1272 errors, they looked something like this. I'm very pleased! I'll go ahead and commit this rule turned off.

C:\Users\path\git\wave-interference\js\diffraction\model\WavingGirlScene.js
  11:8  error  Default import name does not match filename "waving_girl_aperture_png.js"  consistent-default-export-name/default-import-match-filename

C:\Users\path\git\wave-interference\js\diffraction\view\DiffractionScreenView.js
  29:8  error  Default import name does not match filename "waving_girl_icon_png.js"  consistent-default-export-name/default-import-match-filename

C:\Users\path\git\wave-interference\js\interference\InterferenceScreen.js
  13:8  error  Default import name does not match filename "interference_screen_icon_png.js"  consistent-default-export-name/default-import-match-filename

C:\Users\path\git\wave-interference\js\slits\SlitsScreen.js
  13:8  error  Default import name does not match filename "slits_screen_icon_png.js"  consistent-default-export-name/default-import-match-filename

C:\Users\path\git\wave-interference\js\slits\view\PlaneWaveGeneratorNode.js
  15:8  error  Default import name does not match filename "plane_wave_source_png.js"  consistent-default-export-name/default-import-match-filename

C:\Users\path\git\wave-interference\js\waves\WavesScreen.js
  11:8  error  Default import name does not match filename "waves_screen_icon_png.js"  consistent-default-export-name/default-import-match-filename

C:\Users\path\git\wave-interference\js\waves\view\WavesScreenSoundView.js
  15:8  error  Default import name does not match filename "light-beam-loop-v5-eq-out-bass_mp3.js"  consistent-default-export-name/default-import-match-filename
  16:8  error  Default import name does not match filename "speaker-pulse-v4_mp3.js"                consistent-default-export-name/default-import-match-filename
  17:8  error  Default import name does not match filename "water-drop-v5-001_mp3.js"               consistent-default-export-name/default-import-match-filename
  18:8  error  Default import name does not match filename "water-drop-v5-002_mp3.js"               consistent-default-export-name/default-import-match-filename
  19:8  error  Default import name does not match filename "water-drop-v5-003_mp3.js"               consistent-default-export-name/default-import-match-filename
  20:8  error  Default import name does not match filename "water-drop-v5_mp3.js"                   consistent-default-export-name/default-import-match-filename

C:\Users\path\git\wave-interference\js\waves\view\WavesScreenView.js
  34:8  error  Default import name does not match filename "grab_mp3.js"     consistent-default-export-name/default-import-match-filename
  35:8  error  Default import name does not match filename "release_mp3.js"  consistent-default-export-name/default-import-match-filename

C:\Users\path\git\wave-on-a-string\js\wave-on-a-string\view\EndNode.js
  12:8  error  Default import name does not match filename "clamp_png.js"        consistent-default-export-name/default-import-match-filename
  13:8  error  Default import name does not match filename "ring_back_png.js"    consistent-default-export-name/default-import-match-filename
  14:8  error  Default import name does not match filename "ring_front_png.js"   consistent-default-export-name/default-import-match-filename
  15:8  error  Default import name does not match filename "window-back_png.js"  consistent-default-export-name/default-import-match-filename

C:\Users\path\git\wave-on-a-string\js\wave-on-a-string\view\StartNode.js
  26:8  error  Default import name does not match filename "wrench_png.js"  consistent-default-export-name/default-import-match-filename

C:\Users\path\git\wave-on-a-string\js\wave-on-a-string\view\WOASScreenView.js
  27:8  error  Default import name does not match filename "window-front_png.js"  consistent-default-export-name/default-import-match-filename

C:\Users\path\git\waves-intro\js\waves-intro-main.js
  16:8  error  Default import name does not match filename "light_screen_icon_png.js"  consistent-default-export-name/default-import-match-filename
  17:8  error  Default import name does not match filename "sound_screen_icon_png.js"  consistent-default-export-name/default-import-match-filename
  18:8  error  Default import name does not match filename "water_screen_icon_png.js"  consistent-default-export-name/default-import-match-filename

✖ 1272 problems (1272 errors, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

Warning: 1272 errors and 0 warnings Use --force to continue.

Here are the repos that this effects:

• acid-base-solutions
• area-builder
• area-model-common
• arithmetic
• axon
• balancing-act
• balloons-and-static-electricity
• bamboo
• bending-light
• brand
• build-a-molecule
• build-a-nucleus
• build-an-atom
• capacitor-lab-basics
• charges-and-fields
• chipper
• circuit-construction-kit-ac
• circuit-construction-kit-black-box-study
• circuit-construction-kit-common
• circuit-construction-kit-dc
• color-vision
• coulombs-law
• counting-common
• density-buoyancy-common
• density
• dot
• energy-forms-and-changes
• energy-skate-park-basics
• energy-skate-park
• estimation
• expression-exchange
• faradays-law
• fluid-pressure-and-flow
• forces-and-motion-basics
• fourier-making-waves
• fractions-common
• friction
• gene-expression-essentials
• geometric-optics
• graphing-lines
• graphing-quadratics
• gravity-and-orbits
• gravity-force-lab-basics
• gravity-force-lab
• greenhouse-effect
• interaction-dashboard
• inverse-square-law-common
• isotopes-and-atomic-mass
• john-travoltage
• joist
• kite
• make-a-ten
• masses-and-springs-basics
• masses-and-springs
• molarity
• molecule-shapes
• neuron
• number-line-common
• number-line-distance
• number-line-integers
• number-line-operations
• number-play
• ohms-law
• pendulum-lab
• perennial-alias
• perennial
• phet-core
• phet-io-test-sim
• phetcommon
• plinko-probability
• projectile-motion
• proportion-playground
• quadrilateral
• ratio-and-proportion
• resistance-in-a-wire
• rutherford-scattering
• scenery-phet
• scenery
• simula-rasa
• states-of-matter
• tambo
• trig-tour
• utterance-queue
• vegas
• vibe
• wave-interference
• wave-on-a-string
• waves-intro

I'm not really sure how hard it would be to come up with a --fix hack on top of their rule for a one time use. Perhaps that is the way to go instead of manually doing it. That said, likely it is best to rename media files too.

[zepumph]

Ok, commented out above.

[zepumph]

I think it is time to check in with @jessegreenberg. Please let me know what you would like to do in combination with #1060

[samreid]

I'm seeing this error in sim code, even though I have npm install in chipper:

[zepumph]

I don't see a good way to support this, and I'm unsure how to debug it. With that in mind and the feedback over in minseoksuh/eslint-plugin-consistent-default-export-name#7, I think that spinning up our own, similar rule with fewer features makes the most sense. I have something pretty good going in my working copy. Commit coming soon.

[zepumph]

Ok. @jessegreenberg, please review our own implementation of default-import-match-filename.

Using this patch + grunt lint-everything, you can see all the errors. Most likely these will need to be fixed manually.

Index: eslint/.eslintrc.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/eslint/.eslintrc.js b/eslint/.eslintrc.js
--- a/eslint/.eslintrc.js	(revision ceb0be752cff3809647289ea1b6dfe90decd8657)
+++ b/eslint/.eslintrc.js	(date 1630522948197)
@@ -943,10 +943,10 @@
     'no-spaced-func': 'error',
 
     // import variables for sounds and images must have the conventional suffix
-    'import-resource-variable-suffix': 'error'
+    'import-resource-variable-suffix': 'error',
 
     // TODO: comment back in when >1200 errors are fixed, see https://github.com/phetsims/chipper/issues/1079
-    // 'default-import-match-filename': 'error'
+    'default-import-match-filename': 'error'
     ////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
   },
   env: {

What are you feeling about this?

[jessegreenberg]

This lint rule is working well, thanks @zepumph. I just removed a console.log, but I tested it and it is working well.

[zepumph]

Excellent. I'm going to close this issue, and pick this up over in #1060

[jessegreenberg]

Should default-import-match-filename be a total replacement of require-statement-match? It is not catching this currently:

const _ = require( 'lodash' );

Similarly const ChippeerConstants = require( '../common/chipperConstants' ); is not caught. I thought default-import-match-filename might work for all imports. If just for image/audio resources maybe it should be named to resource-require-statement-match or something?

[jessegreenberg]

Nevermind, I see now that default-import-match-filename is for import and require-statement-match is for require statements.