-
Notifications
You must be signed in to change notification settings - Fork 14
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
Determine ESLint rules for TypeScript #1114
Comments
I tested and added a few rules for the plugin. I tried to enable more, but they required reference to tsconfig.json, so I left them commented out for now. The error was:
I tried: 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 1e25f714b57a09f6ee1bbf0a4552a0409595b43c)
+++ b/eslint/.eslintrc.js (date 1634328326966)
@@ -168,7 +168,7 @@
// Not yet enabled because they require tsconfig.json files
// '@typescript-eslint/no-unnecessary-type-assertion':'error',
// '@typescript-eslint/no-unsafe-member-access':'error',
- // '@typescript-eslint/restrict-plus-operands':'error',
+ '@typescript-eslint/restrict-plus-operands':'error',
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
// Best Practices
@@ -971,7 +971,8 @@
},
parserOptions: {
ecmaVersion: 8,
- sourceType: 'module'
+ sourceType: 'module',
+ project: '../chipper/tsconfig/all/tsconfig.json'
},
globals: {
but it failed with errors like:
|
Today during our typescript dev meeting, we decided that we want to make this higher priority to get ts-related lint rules working on our project. This is currently blocking #1203. @jessegreenberg volunteered to help move this forward. Thanks a million. |
I tried to work on this today but got about as far as #1114 (comment). Here are the changes I tried in .eslintrc.js 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 5ae7c8ee6b645bd171be5738a76edc7cad283ad3)
+++ b/package.json (date 1646195788679)
@@ -20,6 +20,7 @@
"@types/qunit": "^2.11.2",
"@types/three": "^0.137.0",
"@typescript-eslint/parser": "5.4.0",
+ "@typescript-eslint/eslint-plugin": "^5.13.0",
"archiver": "^5.3.0",
"axios": "^0.21.4",
"babel-eslint": "^10.1.0",
Index: tsconfig.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tsconfig.json b/tsconfig.json
--- a/tsconfig.json (revision 5ae7c8ee6b645bd171be5738a76edc7cad283ad3)
+++ b/tsconfig.json (date 1646197494006)
@@ -2,6 +2,8 @@
"extends": "./tsconfig-core.json",
"include": [
// Just the code used in sim modules
- "js/getStringModule.js"
+ "js/getStringModule.js",
+
+ "../../axon/js/Action.ts"
]
}
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 5ae7c8ee6b645bd171be5738a76edc7cad283ad3)
+++ b/eslint/.eslintrc.js (date 1646197453836)
@@ -15,9 +15,9 @@
parser: '@typescript-eslint/parser',
- plugins: [
- // '@typescript-eslint'
- ],
+ // plugins: [
+ // // '@typescript-eslint'
+ // ],
// The rules are organized like they are in the list at https://eslint.org/docs/rules/
// First by type, then alphabetically within type
@@ -967,6 +967,32 @@
ecmaVersion: 8,
sourceType: 'module'
},
+ overrides: [
+ {
+ // for files matching this pattern
+ files: '*.ts',
+
+ // the following config will override .js config
+ parser: '@typescript-eslint/parser',
+ parserOptions: {
+ // sourceType: 'module',
+ // ecmaVersion: 2020,
+ project: [
+ '../chipper/tsconfig.json'
+ ]
+ },
+ extends: [ 'plugin:@typescript-eslint/recommended' ],
+ plugins: [
+ '@typescript-eslint'
+ ],
+ rules: {
+ //'@typescript-eslint/no-unnecessary-type-assertion': 'error'
+ '@typescript-eslint/no-unsafe-member-access': 'error'
+ // '@typescript-eslint/restrict-plus-operands':'error',
+ }
+ }
+ ],
+
globals: {
// globals that should never be accessed ---------------------------------
When I lint in a sim repo I see lots of
And when I lint in chipper I see
If the |
I followed the instructions https://www.typescriptlang.org/docs/handbook/typescript-tooling-in-5-minutes.html and https://khalilstemmler.com/blogs/typescript/eslint-for-typescript/ to set up a basic TS project with Here is the .eslintrc
I am going to try linting javascript and typescript together in this simple project. I got js and ts files linting in the same project with rules that use type information with this eslintrc:
I changed the |
Alright, I just got it working by following a recommendation in https://stackoverflow.com/questions/58510287/parseroptions-project-has-been-set-for-typescript-eslint-parser. I added a file chipper/eslint/tsconfig.eslint.json that looks like {
"extends": "../../chipper/tsconfig/all/tsconfig.json",
"include": [
"../../axon/js/*.ts",
"../../quadrilateral/js/**/*"
]
} And made this file the value of
We would have to include all libraries in this new file. |
Here is our patch so far: 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 5ae7c8ee6b645bd171be5738a76edc7cad283ad3)
+++ b/package.json (date 1646253547569)
@@ -20,6 +20,7 @@
"@types/qunit": "^2.11.2",
"@types/three": "^0.137.0",
"@typescript-eslint/parser": "5.4.0",
+ "@typescript-eslint/eslint-plugin": "5.13.0",
"archiver": "^5.3.0",
"axios": "^0.21.4",
"babel-eslint": "^10.1.0",
Index: eslint/tsconfig.eslint.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/eslint/tsconfig.eslint.json b/eslint/tsconfig.eslint.json
new file mode 100644
--- /dev/null (date 1646253271750)
+++ b/eslint/tsconfig.eslint.json (date 1646253271750)
@@ -0,0 +1,7 @@
+{
+ "extends": "../../chipper/tsconfig/all/tsconfig.json",
+ "include": [
+ "../../axon/js/*.ts",
+ "../../quadrilateral/js/**/*"
+ ]
+}
\ No newline at end of file
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 5ae7c8ee6b645bd171be5738a76edc7cad283ad3)
+++ b/eslint/.eslintrc.js (date 1646253667888)
@@ -19,6 +19,26 @@
// '@typescript-eslint'
],
+ "overrides": [
+ {
+ // For .ts files, the following configuration will be used
+ "files": [
+ "**/*.ts"
+ ],
+ "parser": "@typescript-eslint/parser",
+ "parserOptions": {
+ "sourceType": "module",
+ "project": "../chipper/eslint/tsconfig.eslint.json"
+ },
+ "plugins": [
+ "@typescript-eslint"
+ ],
+ "rules": {
+ "@typescript-eslint/restrict-plus-operands": "error"
+ }
+ }
+ ],
+
// 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
JT EDIT: SR and I met to discuss progress and turned #1114 (comment) into a patch. We are both a bit confused about why the new tsconfig file is necessary for the parser, it seems like we should be able to use existing tsconfig files. Together we also tried to move things out of the "overrides" section but hit the "You have used a rule which requires a generated parser..." error. Ill keep seeing if I can learn more. |
Regarding the "failed to load plugin" outside of chipper, maybe this is what we need: https://eslint.org/docs/user-guide/command-line-interface#--resolve-plugins-relative-to. Also see https://eslint.org/docs/user-guide/migrating-to-6.0.0#-plugins-and-shareable-configs-are-no-longer-affected-by-eslints-location EDIT: We already use EDIT: I got this error to go away by using this setting for eslint options. EDIT: Regarding the EDIT: I tried modifying pacakge.json files to override the EDIT; I found that this pattern in tsconfig.eslint.json for EDIT: Patch where things are almost working well. I am worried about the increase in time from this, will test some of that. Index: chipper/package.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/package.json b/chipper/package.json
--- a/chipper/package.json (revision 5ae7c8ee6b645bd171be5738a76edc7cad283ad3)
+++ b/chipper/package.json (date 1646255859875)
@@ -20,6 +20,7 @@
"@types/qunit": "^2.11.2",
"@types/three": "^0.137.0",
"@typescript-eslint/parser": "5.4.0",
+ "@typescript-eslint/eslint-plugin": "5.13.0",
"archiver": "^5.3.0",
"axios": "^0.21.4",
"babel-eslint": "^10.1.0",
Index: chipper/eslint/.eslintignore
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/eslint/.eslintignore b/chipper/eslint/.eslintignore
--- a/chipper/eslint/.eslintignore (revision 5ae7c8ee6b645bd171be5738a76edc7cad283ad3)
+++ b/chipper/eslint/.eslintignore (date 1646316517876)
@@ -13,4 +13,5 @@
../phet-io-website/root/assets/js/QueryStringMachine_1.0.js
../phet-io-website/root/assets/js/QueryStringMachine_2.0.js
../phet-io-website/root/assets/js/phet-io-ga.js
-../chipper/dist
\ No newline at end of file
+../chipper/dist
+*.d.ts
\ No newline at end of file
Index: geometric-optics/js/lens/view/LensScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/geometric-optics/js/lens/view/LensScreenView.ts b/geometric-optics/js/lens/view/LensScreenView.ts
--- a/geometric-optics/js/lens/view/LensScreenView.ts (revision 6462dd2144a7b406fa7e93666d5bedd34c051780)
+++ b/geometric-optics/js/lens/view/LensScreenView.ts (date 1646324631215)
@@ -29,6 +29,8 @@
*/
constructor( model: LensModel, providedOptions: LensScreenViewOptions ) {
+ const x = 5 + '5';
+
const options = optionize<LensScreenViewOptions, {}, GOScreenViewOptions,
'getViewOrigin' | 'createOpticNode'>( {
Index: chipper/eslint/tsconfig.eslint.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/eslint/tsconfig.eslint.json b/chipper/eslint/tsconfig.eslint.json
new file mode 100644
--- /dev/null (date 1646324956920)
+++ b/chipper/eslint/tsconfig.eslint.json (date 1646324956920)
@@ -0,0 +1,6 @@
+{
+ "extends": "../../chipper/tsconfig/all/tsconfig.json",
+ "include": [
+ "../../*/**/*.ts"
+ ]
+}
\ No newline at end of file
Index: chipper/eslint/.eslintrc.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/eslint/.eslintrc.js b/chipper/eslint/.eslintrc.js
--- a/chipper/eslint/.eslintrc.js (revision 5ae7c8ee6b645bd171be5738a76edc7cad283ad3)
+++ b/chipper/eslint/.eslintrc.js (date 1646325842049)
@@ -13,10 +13,31 @@
// in a parent dir
root: true,
- parser: '@typescript-eslint/parser',
+ overrides: [
+ {
+
+ // For .ts files, the following configuration will be used
+ files: [
+ '**/*.ts'
+ ],
+ parser: '@typescript-eslint/parser',
+ parserOptions: {
+ sourceType: 'module',
- plugins: [
- // '@typescript-eslint'
+ // This will be the tsconfig of the repo where lint is being run. Alternatively
+ // a specific tsconfig could be specified in each repo's package.json. Seems to
+ // work for sims but not axon? Getting The file does not match your project config: js\stepTimer.ts.
+ // project: './tsconfig.json'
+ // project: '../chipper/tsconfig/all/tsconfig.json'
+ project: '../chipper/eslint/tsconfig.eslint.json'
+ },
+ plugins: [
+ '@typescript-eslint'
+ ],
+ rules: {
+ '@typescript-eslint/restrict-plus-operands': 'error'
+ }
+ }
],
// The rules are organized like they are in the list at https://eslint.org/docs/rules/
EDIT: Timing impacts of the change: Without
With
EDIT: I think we still need to use the
For the assignment of // Possible supported values for the `aria-live` attributes created in AriaLiveAnnouncer.
class AriaLive extends EnumerationValue {
static POLITE = new AriaLive();
static ASSERTIVE = new AriaLive(); Maybe I should try moving settings out of EDIT: I tried to move all the Progress patch to apply: 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 5ae7c8ee6b645bd171be5738a76edc7cad283ad3)
+++ b/package.json (date 1646330986874)
@@ -20,6 +20,7 @@
"@types/qunit": "^2.11.2",
"@types/three": "^0.137.0",
"@typescript-eslint/parser": "5.4.0",
+ "@typescript-eslint/eslint-plugin": "5.13.0",
"archiver": "^5.3.0",
"axios": "^0.21.4",
"babel-eslint": "^10.1.0",
Index: eslint/.eslintignore
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/eslint/.eslintignore b/eslint/.eslintignore
--- a/eslint/.eslintignore (revision 5ae7c8ee6b645bd171be5738a76edc7cad283ad3)
+++ b/eslint/.eslintignore (date 1646330986853)
@@ -13,4 +13,5 @@
../phet-io-website/root/assets/js/QueryStringMachine_1.0.js
../phet-io-website/root/assets/js/QueryStringMachine_2.0.js
../phet-io-website/root/assets/js/phet-io-ga.js
-../chipper/dist
\ No newline at end of file
+../chipper/dist
+*.d.ts
\ No newline at end of file
Index: eslint/tsconfig.eslint.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/eslint/tsconfig.eslint.json b/eslint/tsconfig.eslint.json
new file mode 100644
--- /dev/null (date 1646330986867)
+++ b/eslint/tsconfig.eslint.json (date 1646330986867)
@@ -0,0 +1,6 @@
+{
+ "extends": "../../chipper/tsconfig/all/tsconfig.json",
+ "include": [
+ "../../*/**/*.ts"
+ ]
+}
\ No newline at end of file
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 5ae7c8ee6b645bd171be5738a76edc7cad283ad3)
+++ b/eslint/.eslintrc.js (date 1646342949339)
@@ -13,10 +13,33 @@
// in a parent dir
root: true,
+ // Without a parser, .js files are linted without es6 transpilation. Use the same parser that we use for TypeScript.
parser: '@typescript-eslint/parser',
- plugins: [
- // '@typescript-eslint'
+ overrides: [
+ {
+
+ // For .ts files, the following configuration will be used
+ files: [
+ '**/*.ts'
+ ],
+ parser: '@typescript-eslint/parser',
+ parserOptions: {
+ sourceType: 'module',
+
+ // Provide a tsconfig so that we can use rules that require type information. tsconfig.eslint.json
+ // gives eslint project information without needing to our actual tsconfig setup.
+ project: '../chipper/eslint/tsconfig.eslint.json'
+ },
+ plugins: [
+ '@typescript-eslint'
+ ],
+ rules: {
+
+ // Put ts specific rules here
+ '@typescript-eslint/restrict-plus-operands': 'error'
+ }
+ }
],
// The rules are organized like they are in the list at https://eslint.org/docs/rules/
Currently I can lint everything except chipper. In that repo I am getting
I tried to add an override to chipper's package.json eslintconfig but it didn't do anything. But I copied the entire "overrides": [
{
"files": [
"**/*.ts"
],
"parser": "@typescript-eslint/parser",
"parserOptions": {
"sourceType": "module",
"project": [
"../chipper/eslint/tsconfig.eslint.json"
]
},
"plugins": [
"@typescript-eslint"
],
"rules": {
"@typescript-eslint/restrict-plus-operands": "error"
}
}
] For some reason, that can be reduced to this to get typescript rules working in chipper "overrides": [
{
"files": [
"**/*.ts"
],
"parser": "@typescript-eslint/parser"
}
] I don't know why that portion has to be duplicated and why it doesn't need Final patch where I can use 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 a304351ca438ae96a53614e83ee2c900c45c6270)
+++ b/package.json (date 1646862574530)
@@ -20,6 +20,7 @@
"@types/qunit": "^2.11.2",
"@types/three": "^0.137.0",
"@typescript-eslint/parser": "5.4.0",
+ "@typescript-eslint/eslint-plugin": "5.13.0",
"archiver": "^5.3.0",
"axios": "^0.21.4",
"babel-eslint": "^10.1.0",
@@ -57,6 +58,14 @@
"parserOptions": {
"sourceType": "module",
"allowImportExportEverywhere": true
- }
+ },
+ "overrides": [
+ {
+ "files": [
+ "**/*.ts"
+ ],
+ "parser": "@typescript-eslint/parser"
+ }
+ ]
}
}
Index: eslint/tsconfig.eslint.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/eslint/tsconfig.eslint.json b/eslint/tsconfig.eslint.json
new file mode 100644
--- /dev/null (date 1646850819065)
+++ b/eslint/tsconfig.eslint.json (date 1646850819065)
@@ -0,0 +1,7 @@
+{
+ "extends": "../../chipper/tsconfig/all/tsconfig.json",
+ "include": [
+ "../../*/**/*.ts",
+ "*.ts"
+ ]
+}
\ No newline at end of file
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 a304351ca438ae96a53614e83ee2c900c45c6270)
+++ b/eslint/.eslintrc.js (date 1646858933586)
@@ -13,10 +13,33 @@
// in a parent dir
root: true,
+ // Without a parser, .js files are linted without es6 transpilation. Use the same parser that we use for TypeScript.
parser: '@typescript-eslint/parser',
- plugins: [
- // '@typescript-eslint'
+ overrides: [
+ {
+
+ // For .ts files, the following configuration will be used
+ files: [
+ '**/*.ts'
+ ],
+ parser: '@typescript-eslint/parser',
+ parserOptions: {
+ sourceType: 'module',
+
+ // Provide a tsconfig so that we can use rules that require type information. tsconfig.eslint.json
+ // gives eslint project information without needing to modify our actual tsconfig setup.
+ project: [ '../chipper/eslint/tsconfig.eslint.json' ]
+ },
+ plugins: [
+ '@typescript-eslint'
+ ],
+ rules: {
+
+ // Put ts specific rules here
+ '@typescript-eslint/restrict-plus-operands': 'error'
+ }
+ }
],
// The rules are organized like they are in the list at https://eslint.org/docs/rules/
|
Ready to bring back to the team. Things that people would have to be aware of before we enable this.
|
Discussed during meeting - We will add the plugin and enable linting of TypeScript. By next week all devs can have that much working. Then in following weeks we will go through as a team (or sub-team) and decide which lint rules to enable. In general we think it is worth the extra time it takes to |
I committed the change but @samreid reported that it isn't working well with WebStorm because WebStorm reports UPDATE: I got it working on both my Windows machines by updating WebStorm to 2021.3. I tried out WebStorm on my mac and doing the steps in #1114 (comment) worked. It did take ~20 seconds or so for WebStorm to recognize UPDATE 2: @zepumph and I saw the patch working well for his machine. I am going to reapply changes. |
The change has been added back and I put out a PSA on slack channels. I also updated phet-info/ide/setup.md with this info. Next step is to decide which lint rules to begin applying, |
Devs are reporting that WebStorm errors with "ESLint: No results for {{file}} after 20 seconds." The recommendation in https://youtrack.jetbrains.com/issue/WEB-38004 is to disable eslint rules that require type information and remove Maybe we can make eslint faster by restricting the files it searches for in |
Could we improve this process by having each repo point to its own tsconfig in parserOptions.project? That way, the type checker only has to check the one sim instead of all 160+ sims? Also, I tested with: extends: [
'plugin:@typescript-eslint/eslint-recommended',
'plugin:@typescript-eslint/recommended'
], and found errors like:
|
Specifying this in package.json for center-and-spread: "eslintConfig": {
"extends": "../chipper/eslint/sim_eslintrc.js",
"parserOptions": {
"project": [
"./tsconfig.json"
]
}
} Changes the time of |
Looking more into this right now. |
I went through all the rules in https://typescript-eslint.io/rules/ and added the rules that were recommended to our eslintrc. Basically this is the same format @samreid did for #814. We should probably do this same criteria for all rules in the plugin, just to see if there are others that we want to add in. When all non-type-checking rules were added in, we had 2942 Problems. There were just a handful of rules with the vast majority of issues. |
Some discussion 4/14/22 The @zepumph collected rules that would be good to enable. Some could be enabled immediately others could not and warrant more discussion. Next time we are going to go through the list and decide which ones we want to enable (and do the work to get the project free of that lint error). |
Investigating 'no-this-alias' errors in #1223. |
@jessegreenberg will create the rest of the issues per rule. Then we are ready to close because type-checking lint rules should be decided on based on the outcome of #1238. Please close when those are made. Thanks @jessegreenberg! |
Great, issues have been made we will continue there. Closing this one. |
so whats the fix at the end?! |
There are ESLint TypeScript rules defined at https://www.npmjs.com/package/@typescript-eslint/eslint-plugin. We should consider which ones we want to use. Note, they can be turned on via:
as described in https://github.com/typescript-eslint/typescript-eslint/blob/master/docs/getting-started/linting/README.md
The text was updated successfully, but these errors were encountered: