Add Typescript eslint #6985

ljowen · 2023-11-27T06:42:01Z

What this PR does

Fixes #3303
Special thanks to @zoran995 for their earlier PR!

Enable eslint for .ts and .tsx files
Run yarn gulp lint --fix
Manually fix remaining problems. Where possible I've been conservative about changing typings or changes that could have unseen side effects.

More rules can be enable progressively

Test me

Given the blast radius exploratory testing is likely best approach along with careful review: http://ci.terria.io/typescript-eslint

Checklist

~~[] There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)~~
~~[ ] I've updated relevant documentation in doc/.~~
I've updated CHANGES.md with what I changed.
I've provided instructions in the PR description on how to test this PR.

.eslintrc

doc/js/rewrite-links.js

lib/ModelMixins/GeojsonMixin.ts

ljowen · 2023-12-04T02:17:31Z

lib/Models/Catalog/CatalogItems/SenapsLocationsCatalogItem.ts

@@ -88,14 +88,27 @@ export class SenapsLocationsStratum extends LoadableStratum(

  static async load(senapsLocationsCatalogItem: SenapsLocationsCatalogItem) {
    const locationsUrl = senapsLocationsCatalogItem._constructLocationsUrl();
+
+    function addStreamIds(f: SenapsFeature, index: number, streamData: any) {


Added streamData parameter instead of relying on closure

ljowen · 2023-12-04T02:19:04Z

lib/Models/Catalog/Esri/ArcGisFeatureServerCatalogItem.ts

Conflict with https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol

ljowen · 2023-12-04T02:24:04Z

lib/Models/Definition/addModelStrataView.ts

@@ -19,6 +19,8 @@ export default function addModelStrataView<
 ): ModelPropertiesFromTraits<InstanceType<T>>;
 export default function addModelStrataView<
  T extends TraitsConstructor<ModelTraits>
+  /* TODO: Fix this overload type */
+  /* eslint-disable-next-line @typescript-eslint/ban-types */
 >(model: Function, Traits: T): ModelPropertiesFromTraits<InstanceType<T>>;


I couldn't easily find an appropriate type for model that would satisfy tsc since it seems to get treated as both a function and a class constructor

ljowen · 2023-12-04T02:26:23Z

lib/ReactViews/Context/ViewStateContext.tsx

+export const withViewState = <P extends WithViewState>(
+  Component: React.ComponentType<P>
+): React.FC<Omit<P, "viewState">> =>
+  function withViewState(props) {


this feels a bit hacky but means we get a displayName

lib/ReactViews/Map/TerriaViewerWrapper/Splitter/dragHook.ts

zoran995 · 2023-12-18T20:06:57Z

.eslintrc

+    "react/jsx-closing-bracket-location": ["error", "line-aligned"],
+    "react/jsx-closing-tag-location": "error",
+    "react/jsx-curly-spacing": ["error", "never", { "allowMultiline": true }],


It would be better to install eslint-config-prettier and let it handle this, I am a bit worried that those rules might cause conflicts with prettier formatting

Prettier recommends using eslint-config-prettier, so we should include this

Thanks @zoran995 I've added that and removed any rules reported by the eslint-config-prettier cli tool: https://github.com/prettier/eslint-config-prettier?tab=readme-ov-file#cli-helper-tool

zoran995 · 2023-12-18T20:07:58Z

.eslintrc

+    "react/no-did-update-set-state": "error",
+    "react/no-will-update-set-state": "error",
+    "react/self-closing-comp": "error",
+    "react/jsx-no-undef": ["error", { "allowGlobals": true }],


I would also consider react/jsx-no-leaked-render and react/no-object-type-as-default-prop as they can early catch some pretty nasty stuff. In the past, there were cases when 0 was rendered due to some programmatic scroll or some weird behaviour

These require an update to eslint-plugin-react and introduce ~100 new errors. I'll create an issue to enable and fix these rules in the interest of keeping the scope of this PR down

zoran995 · 2023-12-18T20:10:10Z

.eslintrc

-      }
-    ],
-    "dot-location": [1, "property"],
-    "eqeqeq": 1,


eqeqeq won't actually be activated by default :(

Thanks for picking this up! I've added it back in and fixed in .ts,.tsx files

zoran995 · 2023-12-18T20:13:18Z

.eslintrc

+        "eslint:recommended",
+        "plugin:react/recommended",
+        "plugin:react-hooks/recommended",
+        "plugin:@typescript-eslint/recommended"


here you also need a plugin:@typescript-eslint/eslint-recommended so js eslint rules are converted to ts version

actually, my bad plugin:@typescript-eslint/recommended will activate it automatically but IMO doesn't hurt to add it explicitly, your call

I'd prefer to be explicit, will add it

zoran995 · 2023-12-18T20:41:08Z

.eslintrc

+    {
+      "files": ["**/*.ts", "**/*.tsx"],
+      "extends": [
+        "eslint:recommended",


To check if this was intentional, having all those extends here will force you to specify every overriding config option in 2 places, above for .js and here for typescript files, i.e. eslint:recommended sets rule-1 to error, you disable it or change it and that here again specify eslint:recommended forcing you to specify config in two places

I've simplified the config so the overrides is just changing to warn errors still be to fixed in ts,tsx files

- Warn instead of ignore new ts rules

…with prettier - Removed problematic overrides as reported by `npx eslint-config-prettier`

- more fixes

ljowen force-pushed the typescript-eslint branch from e816a55 to ce99d9e Compare November 27, 2023 23:16

zoran995 reviewed Nov 28, 2023

View reviewed changes

.eslintrc Outdated Show resolved Hide resolved

zoran995 reviewed Nov 28, 2023

View reviewed changes

.eslintrc Outdated Show resolved Hide resolved

zoran995 reviewed Nov 28, 2023

View reviewed changes

.eslintrc Outdated Show resolved Hide resolved

ljowen force-pushed the typescript-eslint branch 2 times, most recently from a742692 to b2a74e1 Compare November 30, 2023 06:33

ljowen commented Nov 30, 2023

View reviewed changes

doc/js/rewrite-links.js Show resolved Hide resolved

ljowen requested a review from tephenavies December 4, 2023 00:37

ljowen commented Dec 4, 2023

View reviewed changes

lib/ModelMixins/GeojsonMixin.ts Outdated Show resolved Hide resolved

ljowen commented Dec 4, 2023

View reviewed changes

lib/ReactViews/Map/TerriaViewerWrapper/Splitter/dragHook.ts Outdated Show resolved Hide resolved

ljowen force-pushed the typescript-eslint branch 2 times, most recently from 1310353 to 109313a Compare December 4, 2023 04:25

nf-s mentioned this pull request Dec 7, 2023

TerriaJS release 8.4.1 #7005

Closed

ljowen requested a review from zoran995 December 7, 2023 03:52

ljowen mentioned this pull request Dec 7, 2023

Model architecture: Enable eslint for .ts files #3303

Closed

nf-s assigned tephenavies Dec 13, 2023

ljowen mentioned this pull request Dec 13, 2023

Add eslint-plugin-react-hooks linting rule #4565

Closed

zoran995 reviewed Dec 18, 2023

View reviewed changes

ljowen force-pushed the typescript-eslint branch from e47367d to edea374 Compare December 19, 2023 04:33

ljowen added 5 commits December 19, 2023 15:19

Enable typescript linting

5b7223e

Exclude rules where refactor might have side effects

2a5929c

Fix more lint and ts errors

29378b3

Fix remaining lint problems

2ffcbc9

Ignore empty catch block instead of return

5e334ee

ljowen added 12 commits December 19, 2023 15:19

More fixes, extend ts/eslint-recommended at root

283b4b8

- Warn instead of ignore new ts rules

bump ecma version to match tsconfig

006577c

Note eslint ts in CHANGES.md

a179f13

Update lib/ModelMixins/GeojsonMixin.ts

0256727

Update lib/ReactViews/Map/TerriaViewerWrapper/Splitter/dragHook.ts

8a6610f

Undo fixes on .scss.d.ts files

17feacf

Fix timer section

2b0254b

Ignore unused vars/params prefixed with "_"

57b3b6d

update yarn.lock

b892312

Fix lint error in arraysAreEqual.ts

766a584

Add eslint-plugin-prettier to disable lint rules that might conflict …

6184b95

…with prettier - Removed problematic overrides as reported by `npx eslint-config-prettier`

Add eqeqeq rule back and fix in .ts,.tsx

c461a9e

ljowen force-pushed the typescript-eslint branch from 6714541 to 9e2661c Compare December 19, 2023 05:19

Simplify config, only override warnings for typescript files

175eaa5

- more fixes

ljowen force-pushed the typescript-eslint branch from 9e2661c to 175eaa5 Compare December 19, 2023 05:22

Bump warnings count

404d353

tephenavies approved these changes Dec 20, 2023

View reviewed changes

ljowen merged commit e4bc336 into main Dec 20, 2023
5 checks passed

ljowen deleted the typescript-eslint branch December 20, 2023 02:45

zoran995 mentioned this pull request Jan 11, 2024

lint: add eslint to typescript files #6411

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Typescript eslint #6985

Add Typescript eslint #6985

ljowen commented Nov 27, 2023 •

edited

Loading

ljowen Dec 4, 2023

ljowen Dec 4, 2023

ljowen Dec 4, 2023

ljowen Dec 4, 2023

zoran995 Dec 18, 2023

tephenavies Dec 19, 2023

ljowen Dec 19, 2023

zoran995 Dec 18, 2023 •

edited

Loading

ljowen Dec 19, 2023

zoran995 Dec 18, 2023

ljowen Dec 19, 2023 •

edited

Loading

zoran995 Dec 18, 2023

zoran995 Dec 18, 2023

ljowen Dec 19, 2023

zoran995 Dec 18, 2023

ljowen Dec 19, 2023

Add Typescript eslint #6985

Add Typescript eslint #6985

Conversation

ljowen commented Nov 27, 2023 • edited Loading

What this PR does

Test me

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoran995 Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljowen Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljowen commented Nov 27, 2023 •

edited

Loading

zoran995 Dec 18, 2023 •

edited

Loading

ljowen Dec 19, 2023 •

edited

Loading