Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates to linting once typescript-eslint supports Typescript 5.7 #1483

Open
zepumph opened this issue Oct 11, 2024 · 9 comments
Open

Updates to linting once typescript-eslint supports Typescript 5.7 #1483

zepumph opened this issue Oct 11, 2024 · 9 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Oct 11, 2024

Over in #1451, we ended up hacking around a lack of support in typescript-eslint. We are using the unstable_config_lookup_from_file, but typescript-eslint doesn't support it well. Typescript 5.7 will support looking up tsconfig.json t

Support issue: typescript-eslint/typescript-eslint#10115

Once we have good support, we are going to be able to use more than one eslint.config.mjs file per repo. This will greatly reduce the complexity of flat config files in repos that lint browser and node code.

  1. aqua and chipper are the only repos using this feature at this time.
  2. Add an eslint.mjs file at each level to specify what linting config is needed (aqua/js/grunt/ will use node, but aqua/js/report will use a browser config).
  3. Delete all the "getXConfiguration()" functions in common eslint config files, and only support exporting default with an spread super config (...rootEslintConfig).
  4. We don't think that this will involve any changes to our "sub-directory" tsconfig files. We don't think lint and webstorm know how to start at a file, and then navigate through a reference path at the root tsconfig to find the right config via a project reference. Let's give that test one more check in 5.7, just to make sure that we need this complexity, but we don't expect any changes.
@zepumph
Copy link
Member Author

zepumph commented Oct 28, 2024

It isn't clear that this ISN'T supported by typescript eslint 8.10 (which supports typescript 5.6). I think we should upgrade to 8.11 (current as of last week), and see if the behavior of muiltiple eslint.config.mjs files up the hierarchy resolves correctly.

@zepumph
Copy link
Member Author

zepumph commented Nov 4, 2024

Ok. We were confused. This is a feature of Typescript 5.7. Still on hold until typescript and typescript-eslint supports that.

@zepumph
Copy link
Member Author

zepumph commented Nov 26, 2024

@samreid and I believe this is now unblocked from phetsims/perennial#417. I'm going to put that issue on hold until we figure out the best way to have tsconfig and eslint config files in perennial and chipper.

@samreid
Copy link
Member

samreid commented Nov 27, 2024

We found that we were getting thwarted by the default type roots, and we have to opt out of that to be able to have sensible/accurate type checking:

Subject: [PATCH] Reduce prism count during fuzzing, see https://github.com/phetsims/bending-light/issues/423
---
Index: tsconfig/tsconfig-browser.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tsconfig/tsconfig-browser.json b/tsconfig/tsconfig-browser.json
--- a/tsconfig/tsconfig-browser.json	(revision 6f4c4727b2ed27019e1b98f884483723e26110be)
+++ b/tsconfig/tsconfig-browser.json	(date 1732751819235)
@@ -8,11 +8,17 @@
   "extends": "./tsconfig-core.json",
   "compilerOptions": {
     /* Allow accessing UMD globals from modules. */
-    "allowUmdGlobalAccess": true
+    "allowUmdGlobalAccess": true,
+
+    // Override the default behavior that discovers all types from node_modules/@types
+    "typeRoots": [
+    ]
   },
   "files": [
     "../js/phet-types.d.ts",
     "../js/phet-types-module.d.ts",
+
+    // TODO: Can we use typeRoots instead of files? See https://github.com/phetsims/chipper/issues/1483
     "../node_modules/@types/jquery/index.d.ts",
     "../node_modules/@types/lodash/index.d.ts",
     "../node_modules/@types/p2/index.d.ts",

@samreid
Copy link
Member

samreid commented Nov 28, 2024

Messy exploratory patch

Subject: [PATCH] Reduce prism count during fuzzing, see https://github.com/phetsims/bending-light/issues/423
---
Index: chipper/js/common/tsconfig.json
===================================================================
diff --git a/chipper/js/common/tsconfig.json b/chipper/js/common/tsconfig.json
deleted file mode 100644
--- a/chipper/js/common/tsconfig.json	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
+++ /dev/null	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
@@ -1,9 +0,0 @@
-{
-  "extends": "../../tsconfig-chipper-node.json",
-  // this reference needs to be duplicated with the super config, because references to not extend
-  "references": [
-    {
-      "path": "../../tsconfig-chipper.json"
-    }
-  ]
-}
\ No newline at end of file
Index: chipper/js/data/tsconfig.json
===================================================================
diff --git a/chipper/js/data/tsconfig.json b/chipper/js/data/tsconfig.json
deleted file mode 100644
--- a/chipper/js/data/tsconfig.json	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
+++ /dev/null	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
@@ -1,9 +0,0 @@
-{
-  "extends": "../../tsconfig-chipper-node.json",
-  // this reference needs to be duplicated with the super config, because references to not extend
-  "references": [
-    {
-      "path": "../../tsconfig-chipper.json"
-    }
-  ]
-}
\ No newline at end of file
Index: chipper/js/grunt/tsconfig.json
===================================================================
diff --git a/chipper/js/grunt/tsconfig.json b/chipper/js/grunt/tsconfig.json
deleted file mode 100644
--- a/chipper/js/grunt/tsconfig.json	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
+++ /dev/null	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
@@ -1,9 +0,0 @@
-{
-  "extends": "../../tsconfig-chipper-node.json",
-  // this reference needs to be duplicated with the super config, because references to not extend
-  "references": [
-    {
-      "path": "../../tsconfig-chipper.json"
-    }
-  ]
-}
\ No newline at end of file
Index: chipper/js/phet-io/tsconfig.json
===================================================================
diff --git a/chipper/js/phet-io/tsconfig.json b/chipper/js/phet-io/tsconfig.json
deleted file mode 100644
--- a/chipper/js/phet-io/tsconfig.json	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
+++ /dev/null	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
@@ -1,9 +0,0 @@
-{
-  "extends": "../../tsconfig-chipper-node.json",
-  // this reference needs to be duplicated with the super config, because references to not extend
-  "references": [
-    {
-      "path": "../../tsconfig-chipper.json"
-    }
-  ]
-}
\ No newline at end of file
Index: chipper/js/scripts/tsconfig.json
===================================================================
diff --git a/chipper/js/scripts/tsconfig.json b/chipper/js/scripts/tsconfig.json
deleted file mode 100644
--- a/chipper/js/scripts/tsconfig.json	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
+++ /dev/null	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
@@ -1,9 +0,0 @@
-{
-  "extends": "../../tsconfig-chipper-node.json",
-  // this reference needs to be duplicated with the super config, because references to not extend
-  "references": [
-    {
-      "path": "../../tsconfig-chipper.json"
-    }
-  ]
-}
\ No newline at end of file
Index: chipper/tsconfig-chipper-browser.json
===================================================================
diff --git a/chipper/tsconfig-chipper-browser.json b/chipper/tsconfig-chipper-browser.json
deleted file mode 100644
--- a/chipper/tsconfig-chipper-browser.json	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
+++ /dev/null	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
@@ -1,16 +0,0 @@
-{
-  "extends": "../perennial-alias/tsconfig/tsconfig-browser.json",
-  "include": [
-
-    // top level only
-    "js/*",
-
-    // and the tests
-    "js/browser/sim-tests/**/*"
-  ],
-  "references": [
-    {
-      "path": "../perennial-alias/tsconfig/buildjson/tsconfig.json"
-    }
-  ]
-}
\ No newline at end of file
Index: chipper/tsconfig-chipper-node.json
===================================================================
diff --git a/chipper/tsconfig-chipper-node.json b/chipper/tsconfig-chipper-node.json
deleted file mode 100644
--- a/chipper/tsconfig-chipper-node.json	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
+++ /dev/null	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
@@ -1,19 +0,0 @@
-{
-  "extends": "../perennial-alias/tsconfig/tsconfig-node.json",
-  "include": [
-    "js/common/**/*",
-    "js/data/**/*",
-    "js/grunt/**/*",
-    "js/phet-io/**/*",
-    "js/scripts/**/*",
-    "test/**/*",
-    "eslint/**/*",
-    // Gruntfile and eslint.config.mjs
-    "*"
-  ],
-  "references": [
-    {
-      "path": "./tsconfig-chipper.json"
-    }
-  ]
-}
\ No newline at end of file
Index: density/eslint.config.mjs
===================================================================
diff --git a/density/eslint.config.mjs b/density/eslint.config.mjs
deleted file mode 100644
--- a/density/eslint.config.mjs	(revision ee0ad5ed10f7fcd3b6660609f5176e18734d7b7a)
+++ /dev/null	(revision ee0ad5ed10f7fcd3b6660609f5176e18734d7b7a)
@@ -1,16 +0,0 @@
-// Copyright 2024, University of Colorado Boulder
-
-/**
- * ESLint configuration for density.
- *
- * @author Sam Reid (PhET Interactive Simulations)
- * @author Michael Kauzmann (PhET Interactive Simulations)
- */
-
-import simEslintConfig from '../perennial-alias/js/eslint/config/sim.eslint.config.mjs';
-import banTSCommentConfig from '../perennial-alias/js/eslint/config/util/banTSCommentConfig.mjs';
-
-export default [
-  ...simEslintConfig,
-  ...banTSCommentConfig
-];
\ No newline at end of file
Index: chipper/tsconfig.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/tsconfig.json b/chipper/tsconfig.json
--- a/chipper/tsconfig.json	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
+++ b/chipper/tsconfig.json	(date 1732748099335)
@@ -13,14 +13,36 @@
  * @author Michael Kauzmann (PhET Interactive Simulations)
  * @author Sam Reid (PhET Interactive Simulations)
  */
+//{
+//  "files": [],
+//  "references": [
+//    {
+//      "path": "./tsconfig-chipper-node.json"
+//    },
+//    {
+//      "path": "./js/browser/tsconfig.json"
+//    }
+//  ]
+//}
+
 {
-  "files": [],
+  "extends": "../perennial-alias/tsconfig/tsconfig-node.json",
+  "include": [
+    "js/**/*",
+    "test/**/*",
+    // Gruntfile and eslint.config.mjs
+    "*"
+  ],
+  "exclude": [
+    "js/browser-and-node/**/*",
+    "js/browser/**/*"
+  ],
   "references": [
     {
-      "path": "./tsconfig-chipper-node.json"
+      "path": "./tsconfig-chipper.json"
     },
     {
-      "path": "./tsconfig-chipper-browser.json"
+      "path": "./js/browser/tsconfig.json"
     }
   ]
 }
\ No newline at end of file
Index: chipper/js/browser/LocalizedString.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/js/browser/LocalizedString.ts b/chipper/js/browser/LocalizedString.ts
--- a/chipper/js/browser/LocalizedString.ts	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
+++ b/chipper/js/browser/LocalizedString.ts	(date 1732748783688)
@@ -19,6 +19,11 @@
 // constants
 const FALLBACK_LOCALE = 'en';
 
+const n: number = 'seven';
+console.log( n );
+
+// process.exit( 0 );
+
 // for readability/docs
 type TranslationString = string;
 export type LocalizedStringStateDelta = Partial<Record<Locale, TranslationString>>;
Index: density/js/compare/CompareScreen.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density/js/compare/CompareScreen.ts b/density/js/compare/CompareScreen.ts
--- a/density/js/compare/CompareScreen.ts	(revision ee0ad5ed10f7fcd3b6660609f5176e18734d7b7a)
+++ b/density/js/compare/CompareScreen.ts	(date 1732749011945)
@@ -19,6 +19,8 @@
 import density from '../density.js';
 import DensityStrings from '../DensityStrings.js';
 
+
+process.exit( 0 );
 export default class CompareScreen extends Screen<DensityCompareModel, DensityCompareScreenView> {
   public constructor( tandem: Tandem ) {
     const icon = DensityBuoyancyScreenView.getThreeIcon( compare_screen_icon_png, () => getDensityCompareIcon() );
Index: chipper/js/browser/tsconfig.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/js/browser/tsconfig.json b/chipper/js/browser/tsconfig.json
--- a/chipper/js/browser/tsconfig.json	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
+++ b/chipper/js/browser/tsconfig.json	(date 1732748607455)
@@ -1,5 +1,8 @@
 {
-  "extends": "../../tsconfig-chipper-browser.json",
+  "extends": "../../../perennial-alias/tsconfig/tsconfig-browser.json",
+  "include": [
+    "**/*"
+  ],
   // this reference needs to be duplicated with the super config, because references to not extend
   "references": [
     {
Index: chipper/tsconfig-chipper.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/tsconfig-chipper.json b/chipper/tsconfig-chipper.json
--- a/chipper/tsconfig-chipper.json	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
+++ b/chipper/tsconfig-chipper.json	(date 1732748238619)
@@ -1,3 +1,4 @@
+// TODO: help
 {
   "extends": "../perennial-alias/tsconfig/tsconfig-node.json",
   // Entry points for checked out resources that may not be on main, but need to version together. These must be usable
Index: aqua/js/browser-tools/fuzz-lightyear.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/aqua/js/browser-tools/fuzz-lightyear.ts b/aqua/js/browser-tools/fuzz-lightyear.ts
--- a/aqua/js/browser-tools/fuzz-lightyear.ts	(revision 355f63df0c9f093a62582d0ec7ab28ecf0c9e6aa)
+++ b/aqua/js/browser-tools/fuzz-lightyear.ts	(date 1732751045859)
@@ -9,6 +9,8 @@
 
   window.assertions.enableAssert();
 
+  process.exit(0);
+
   // Grab all query parameters to pass to the simulation, and add additional ones for receiving messages.
   let simulationQueryString = window.location.search;
 
Index: aqua/package.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/aqua/package.json b/aqua/package.json
--- a/aqua/package.json	(revision 355f63df0c9f093a62582d0ec7ab28ecf0c9e6aa)
+++ b/aqua/package.json	(date 1732751411214)
@@ -12,7 +12,6 @@
   },
   "devDependencies": {
     "@slack/bolt": "~3.11.0",
-    "dotenv": "^16.0.3",
     "grunt": "~1.5.3",
     "lodash": "~4.17.10"
   },
Index: chipper/test/chainsBuildTest.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/test/chainsBuildTest.ts b/chipper/test/chainsBuildTest.ts
--- a/chipper/test/chainsBuildTest.ts	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
+++ b/chipper/test/chainsBuildTest.ts	(date 1732748430724)
@@ -13,6 +13,11 @@
 
 qunit.module( 'Chains building' );
 
+const n: number = 'seven';
+console.log( n );
+
+process.exit( 0 );
+
 function assertFileExistence( assert: Assert, filename: string ): void {
   assert.ok( grunt.file.exists( filename ), filename );
 }
Index: chipper/tsconfig/buildtools/tsconfig.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/tsconfig/buildtools/tsconfig.json b/chipper/tsconfig/buildtools/tsconfig.json
--- a/chipper/tsconfig/buildtools/tsconfig.json	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
+++ b/chipper/tsconfig/buildtools/tsconfig.json	(date 1732748238602)
@@ -1,3 +1,5 @@
+// TODO: help
+
 {
   // Chipper and its node dependencies is what we call "buildtools"
   "compilerOptions": {
Index: bending-light/js/prisms/view/PrismNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/bending-light/js/prisms/view/PrismNode.ts b/bending-light/js/prisms/view/PrismNode.ts
--- a/bending-light/js/prisms/view/PrismNode.ts	(revision 0feb145a2a0b094d92c478228fe6aa0e22d5944c)
+++ b/bending-light/js/prisms/view/PrismNode.ts	(date 1732770207095)
@@ -112,7 +112,7 @@
       prismsModel.removePrism( prism );
       prism.shapeProperty.unlink( this.updatePrismShape );
       prism.positionProperty.unlink( this.updatePrismShape );
-      dragBoundsProperty.unlink( keepInBounds );
+      // dragBoundsProperty.unlink( keepInBounds );
       prismsModel.prismMediumProperty.unlink( this.updatePrismColor );
       prismLayer.removeChild( this );
 
@@ -131,7 +131,7 @@
     } );
 
     // When the window reshapes, make sure no prism is left outside of the play area
-    dragBoundsProperty.lazyLink( keepInBounds );
+    // dragBoundsProperty.lazyLink( keepInBounds );
 
     if ( !isIcon ) {
       prismPathNode.addInputListener( this.dragListener );
@@ -172,7 +172,7 @@
     prism.shapeProperty.link( this.updatePrismShape );
     prism.positionProperty.link( this.updatePrismShape );
 
-    // used in PrismToolboxNode
+    // used in PrismToolboxNode // TODO: probably got broken during typescript refactor
     this.updatePrismColor = () => {
       const indexOfRefraction = prismsModel.prismMediumProperty.value.substance.indexOfRefractionForRedLight;
 
Index: chipper/js/browser/chipper.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/js/browser/chipper.ts b/chipper/js/browser/chipper.ts
--- a/chipper/js/browser/chipper.ts	(revision 85e301688249794d5aff05b52a433420d2b6dbcf)
+++ b/chipper/js/browser/chipper.ts	(date 1732755576959)
@@ -8,5 +8,10 @@
 
 import Namespace from '../../../phet-core/js/Namespace.js';
 
+const n: number = 'seven';
+console.log( n );
+
+// process.exit( 0 );
+
 // NOTE: special logic in Namespace to allow register to work!
 export default new Namespace( 'chipper' );
\ No newline at end of file
Index: perennial-alias/tsconfig/tsconfig-browser.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/perennial-alias/tsconfig/tsconfig-browser.json b/perennial-alias/tsconfig/tsconfig-browser.json
--- a/perennial-alias/tsconfig/tsconfig-browser.json	(revision 6f4c4727b2ed27019e1b98f884483723e26110be)
+++ b/perennial-alias/tsconfig/tsconfig-browser.json	(date 1732751819235)
@@ -8,11 +8,17 @@
   "extends": "./tsconfig-core.json",
   "compilerOptions": {
     /* Allow accessing UMD globals from modules. */
-    "allowUmdGlobalAccess": true
+    "allowUmdGlobalAccess": true,
+
+    // Override the default behavior that discovers all types from node_modules/@types
+    "typeRoots": [
+    ]
   },
   "files": [
     "../js/phet-types.d.ts",
     "../js/phet-types-module.d.ts",
+
+    // TODO: Can we use typeRoots instead of files? See https://github.com/phetsims/chipper/issues/1483
     "../node_modules/@types/jquery/index.d.ts",
     "../node_modules/@types/lodash/index.d.ts",
     "../node_modules/@types/p2/index.d.ts",

@zepumph zepumph self-assigned this Dec 2, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Dec 2, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Dec 2, 2024
zepumph added a commit to phetsims/quake that referenced this issue Dec 2, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Dec 2, 2024
zepumph added a commit to phetsims/quake that referenced this issue Dec 2, 2024
zepumph added a commit to phetsims/rosetta that referenced this issue Dec 2, 2024
zepumph added a commit to phetsims/aqua that referenced this issue Dec 2, 2024
@zepumph
Copy link
Member Author

zepumph commented Dec 2, 2024

Ok, I believe that most everything is handled here, but there was one awkward part, where we sometimes use paths that assume that all configs are at the top of a repo, and when they are in a nested dir, they don't work. I added TODOs for specificity.

The only gross function we still have is getNodeConfiguration. I think this is a good time to check in with @samreid.

@samreid
Copy link
Member

samreid commented Dec 3, 2024

We found that the floating promises and the ignores in root.eslint.config.mjs suffer from this problem. The floating promises can be added in specific directories, but we may want a way to strip out the ignores when you end up extending root.eslint.config.mjs anyplace other than the top level of a repo. We can decide where to apply that filter.

@samreid samreid removed their assignment Dec 3, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Dec 3, 2024
@zepumph
Copy link
Member Author

zepumph commented Dec 3, 2024

Ok. I believe this issue is close, but I ran into a bug (I think) in webstorm that seems like nested eslint configs aren't working for typescript files and lint config in grunt/tasks. I added TODOs. I found that skiffle's js files in grunt/tasks/ were working though. Not sure how to proceed, but I committed anyways since command line linting is working. I hope @samreid and help me get past this. Basically look for a TODO, and uncomment the very incorrect lint and see no lint failures in webstorm, but failures reported in the command line.

@zepumph zepumph assigned samreid and unassigned zepumph Dec 4, 2024
@samreid
Copy link
Member

samreid commented Dec 4, 2024

I have tried:

I have no leads. Would be good to collaborate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants