Lint overrides do not flag ban-ts-comment

[samreid]

From phetsims/phet-core#130 (comment), @pixelzoom said:

@samreid I'm confused about how you were able to add @ts-expect-error comments to geometric-optics, natural-selection, ph-scale, and scenery-phet. All of those repos add ESlint rule "@typescript-eslint/ban-ts-comment" in package.json, which should prohibit @ts-expect-error comments. Thoughts on why this did not fail?

I'm seeing the same problem. I tested adding this ts-expect-error in geometric optics:

Subject: [PATCH] Fix optionize type parameters, see https://github.com/phetsims/phet-core/issues/130
---
Index: js/geometric-optics-main.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/geometric-optics-main.ts b/js/geometric-optics-main.ts
--- a/js/geometric-optics-main.ts	(revision 6e5be01a615db03a8583136f35783a43b2bb5062)
+++ b/js/geometric-optics-main.ts	(date 1682077217077)
@@ -16,4 +16,8 @@
     phetioDesigned: true
   } );
   sim.start();
+
+  // @ts-expect-error
+  const x: number = 'seven';
+  console.log( x );
 } );

And I confirmed it has "@typescript-eslint/ban-ts-comment": "error" in the package.json. Lint is not catching the ts-expect-error. I saw that it overrides no-explicit-any and that correctly catches any types.

I also saw that changing package.json like so correctly catches the errors:

Subject: [PATCH] Fix optionize type parameters, see https://github.com/phetsims/phet-core/issues/130
---
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 6e5be01a615db03a8583136f35783a43b2bb5062)
+++ b/package.json	(date 1682077869621)
@@ -39,7 +39,15 @@
         ],
         "rules": {
           "@typescript-eslint/no-explicit-any": "error",
-          "@typescript-eslint/ban-ts-comment": "error"
+          "@typescript-eslint/ban-ts-comment": [
+            "error",
+            {
+              "ts-ignore": true,
+              "ts-check": true,
+              "ts-expect-error": true,
+              "ts-nocheck": true
+            }
+          ]
         }
       }
     ]

So my current hypothesis is that lint has trouble overriding a "structured" value. I also wonder if this may be corrected in a newer version of lint.

NOTE: A reminder to disable lint cache while experimenting with rule changes: grunt lint --disable-eslint-cache

@pixelzoom how do you recommend to proceed?

[pixelzoom]

I guess we need to either try a newer version of lint, or add the above patch to package.json. Or investigate further in case there's something we don't know about happening here. Note that @zepumph recommended this package.json approach in #1380, so might want to consult with him.

[zepumph]

Ahh good catch. Yes the override would need to have the right options provided to it, otherwise it is just going to use the defaults:

from https://typescript-eslint.io/rules/ban-ts-comment/:

const defaultOptions: Options = [
  {
    "ts-expect-error": "allow-with-description",
    "ts-ignore": true,
    "ts-nocheck": true,
    "ts-check": false,
    minimumDescriptionLength: 3,
  },
];

In other news, we should probably update our whole project to the allow-with-description default.

2 paths forward:

1. Duplicate the desired schema in all location where you want to prohibit all ts-expect-errors
2. Create a config file in chipper/eslint/ called "banAllTSComments", and you can use multiple inheritance in the extends value (instead of just a string):

banAllTSComments_eslintrc.js

module.exports = {
  overrides: [
    {
      files: [
        '**/*.ts'
      ],
      rules: {
        '@typescript-eslint/ban-ts-comment': [
          'error',
          {
            'ts-ignore': true,
            'ts-check': true,
            'ts-expect-error': true,
            'ts-nocheck': true
          }
        ]
      }
    }
  ]
};

And the GO package would look like:

  "eslintConfig": {
    "extends": [
      "../chipper/eslint/sim_eslintrc.js",
      "../chipper/eslint/banAllTSComments_eslintrc.js"
    ],

Either way is fine with me. I'd probably factor it out, but also if I was @pixelzoom, often adding many extra rules into most of my sims, I'd probably have factored all of them out into a "superStrict_eslintrc.js" for reuse.

Hope that's helpful.

[pixelzoom]

ban-ts-comment appears in 28 package.json files. keplers-law is the only occurence that I did not create.

[pixelzoom]

@zepumph said:

... I'd probably factor it out, but also if I was @pixelzoom, often adding many extra rules into most of my sims, I'd probably have factored all of them out into a "superStrict_eslintrc.js" for reuse.

That's problematic, because I typically add additional rules incrementally, especially when it's a JavaScript-to-TypeScript conversion.

I'm leaning towards duplicating the schema, via this change in package.json files:

- "@typescript-eslint/ban-ts-comment": "error"
+ "@typescript-eslint/ban-ts-comment": [ "error", {
     "ts-expect-error": true,
     "ts-ignore": true,
     "ts-check": true,
     "ts-nocheck": true
   } ]

[samreid]

I agree with the recommendation in the preceding recommendation. @zepumph sound OK to you? Who should implement?

[pixelzoom]

To clarify...

27 of the 28 occurrences of ban-ts-comment in package.json were added by me, and I can confirm that what I proposed in #1389 (comment) is appropriate. Feel free to add them, or wait until I can do it.

1 occurrence is in keplers-laws. I don't know who added it, or what's appropriate for that repo. So the responsible developer should be consulted, or at least notified.

[pixelzoom]

27 of the 28 occurrences of ban-ts-comment in package.json were added by me, and I can confirm that what I proposed in #1389 (comment) is appropriate. ...

Correction... scenery-phet has an unresolved ts-expect-error comment, see phetsims/scenery-phet#808. So it will need something like:

- "@typescript-eslint/ban-ts-comment": "error"
+ "@typescript-eslint/ban-ts-comment": [ "error", {
     "ts-expect-error": "allow-with-description",
     "ts-ignore": true,
     "ts-check": true,
     "ts-nocheck": true
   } ]

[pixelzoom]

In the above commits, I configured ban-ts-comment is all package.json files where it appears.

@AgustinVallejo @jonathanolson FYI, there was 1 ts-expect-error is keplers-laws, in FirstLawGraph.ts. I added a description:

      // @ts-expect-error eccentricities should be changed to a Map
      const value = eccentricities[ eccentricitiesKey ];

... and configured keplers-laws/package.json like this:

          "@typescript-eslint/ban-ts-comment": [
            "error",
            {
              "ts-expect-error": "allow-with-description",
              "ts-ignore": true,
              "ts-check": true,
              "ts-nocheck": true
            }
          ]

@AgustinVallejo or @jonathanolson after reviewing this change in keplers-law, you can close this isssue.

[AgustinVallejo]

Closing as that comment is no longer present in keplers laws