Lint fixes #7272

ljowen · 2024-09-17T04:17:15Z

What this PR does

Fixes 215 remaining lint warnings, mostly unused params, functions and constants.

Add eslint-disable comments for:
- no-unsafe-declaration-merging https://typescript-eslint.io/rules/no-unsafe-declaration-merging/ due to clash with TS2611 type errors (see UrlTraits.ts for example)
- react-hooks/exhaustive-deps as these will most likely have side effects so will need more testing / work to enable / workout which uses are valid

Test me

Verify tests pass and app working as expected
yarn gulp lint should return 0 warnings

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.

lib/Models/BoxDrawing.ts

lib/ReactViews/Map/MapNavigation/Items/Compass/Compass.tsx

lib/ReactViews/SelectableDimensions/Color.tsx

ljowen · 2024-09-17T05:04:11Z

lib/Core/animation.ts

@@ -33,10 +33,10 @@ const transitionEnd = (element: Element | null) =>
  });

 const animationTimeout = (
-  timeoutID: ReturnType<typeof setTimeout> | undefined
+  _timeoutID: ReturnType<typeof setTimeout> | undefined


There's a lot of unused params across the codebase, we could instead just turn off the rule for unused params, open to suggestions (I prefer having to be explicit)

As someone who isn't intimately familiar with the code base, it's a one time cost to prepend a "_" to (at most) 215 places, and that prevents future unused leftovers that will confuse me.

zoran995 · 2024-09-17T08:05:02Z

.eslintrc

-        "@typescript-eslint/no-unsafe-declaration-merging": "warn",
-        "react-hooks/exhaustive-deps": "warn",
+        "@typescript-eslint/no-unsafe-declaration-merging": "off",
+        "react-hooks/exhaustive-deps": "off",


I would vote for leaving this turned on, don't how much of them is there but definitely better would be to suppress them using eslint-disable comments. If I recall correctly there isn't much of hook usages.
p.s. Yeah it is bad practice to suppress the warning or error but would always choose that instead of disabling the rule for all future code.

My thinking was just having a smaller diff (and I'd like to resolve the exhaustive-deps eventually) but you're right better to use disable comments to prevent further instances. Will update

.eslintrc

test/Models/Catalog/CatalogItems/GeoJsonCatalogItemSpec.ts

pjonsson · 2024-09-19T07:25:35Z

lib/Table/TableColumn.ts

@@ -920,6 +920,6 @@ function toNumber(value: string): number | null {
  return null;
 }

-function nullFunction(rowIndex: number) {
+function nullFunction() {


This function is returned from scaledValueFunctionForType, doesn't callers of that function expect the returned function to accept one parameter?

They do but it doesn't look like typescript minds: example . Happy to change it to (_rowIndex: number) if you think that's clearer

I had a second look, and wrote something that roughly equals "the type signature states the returned function should take a number and return a number, so I suspect removing the parameter will cause problems in some future upgrade of TypeScript".

As a side note, the return type of nullFunction is null rather than number, so the type is already wrong in the current code base. The function body is just return null, so TypeScript must be able to infer that return type as well.

I feel this behavior of a type checker is really strange, so I had a third look. The function getScaledValueFunctionForType was added in #3594 and had a caller in TableMixin.ts, but that caller no longer seems to be present. Could getScaledValueFunctionForType (and its helper functions) be removed instead?

Nice find thanks I'll remove it

pjonsson · 2024-09-19T09:11:38Z

If you are focusing on lint-related things in the near future, it might be worth considering #7150 after this PR has been merged (I'll fix the conflicts after this is merged).

pjonsson · 2024-09-23T11:56:58Z

lib/Models/Catalog/CatalogReferences/ThreddsItemReference.ts

@@ -121,7 +121,7 @@ export default class ThreddsItemReference extends UrlMixin(
    );
    if (model === undefined) return;
    this.setThreddsStrata(model);
-    previousTarget = model;
+    _previousTarget = model;


What do these assignments do, is previousTarget some kind of return-value type so the caller gets a hold of a reference to the model?

That's my understanding, it's updating the reference. Although looking where it's call in ReferenceMixin that reference doesn't appear to be further used. @na9da do you have insight into this?

I think that line has no effect and can be removed. There's couple more of these.

pjonsson · 2024-09-23T12:02:31Z

lib/Models/Definition/LoadableStratum.ts

@@ -24,7 +24,7 @@ export default function LoadableStratum<
        get: function () {
          return undefined;
        },
-        set: function (value: any) {
+        set: function () {


Same comment for this as the nullFunction below: I suspect removing the parameter will cause problems in some future TypeScript upgrade (and a setter without parameter is strange).

Good point, will update

pjonsson · 2024-09-23T12:05:13Z

lib/Models/Leaflet.ts

@@ -63,7 +63,7 @@ import Terria from "./Terria";
 // We want TS to look at the type declared in lib/ThirdParty/terriajs-cesium-extra/index.d.ts
 // and import doesn't allows us to do that, so instead we use require + type casting to ensure
 // we still maintain the type checking, without TS screaming with errors
-const FeatureDetection: FeatureDetection =
+const _FeatureDetection: FeatureDetection =


This looks like something special that isn't just an unused parameter, can @ts-expect-error be used here so this variable doesn't get lost among the noise of unused parameters?

Scratch my previous remark, the comment in the source code is 4+ years old. In #6944, the FeatureDetection variable is removed from CorsProxy.ts since the FeatureDetection is not used in that file any longer, so this line corresponds to an unused import as far as I understand. I'm guessing the variable and the comment can be removed?

pjonsson · 2024-09-23T12:35:56Z

test/Models/Catalog/Ows/WebProcessingServiceCatalogFunctionSpec.ts

@@ -148,7 +148,7 @@ describe("WebProcessingServiceCatalogFunction", function () {
      let dispose: any;
      job = (await wps.submitJob()) as WebProcessingServiceCatalogFunctionJob;

-      await new Promise<void>((resolve, reject) => {
+      await new Promise<void>((resolve, _reject) => {


The change in YDYRCatalogFunctionSpec above just removes the parameter. If this isn't somehow different from that, I think it's better to either choose to always keep the unused reject with an underscore or always remove it.

I'll remove the unused rejects since it's not communicating anything extra

pjonsson · 2024-09-23T12:36:56Z

test/Models/Experiment.ts

@@ -49,7 +49,7 @@ describe("NewStuff", function () {
      );
    });

-    autorun((dispose) => {
+    autorun((_dispose) => {


Did you intend to remove the console.log in the next line too?

This whole file isn't actually asserting anything and hasn't been touched since 2021, I'll check with the team about removing it completely.

CLAassistant · 2024-09-24T02:40:10Z

All committers have signed the CLA.

- remove unneeded console.logs

Turn off exhaustive-deps as fixing these are likely to have side effects

…oks/exhaustive-deps": "error", Disable existing usages via comment

- Delete Experiment.ts

pjonsson

I had a look again and while I don't speak TypeScript fluently, from what I can see this looks like a solid improvement.

Not having 200+ lint warnings to sift through for every commit will make development easier going forward, so I definitely want this (or some alternative) merged as soon as possible.

na9da

@ljowen looks good to me. Just the one comment on removing the _previousTarget = model lines. I also concur with @pjonsson and @zoran995 on not turning off the lint rules so that future code is checked. Please update the PR description to reflect that change.

Spotted in TerriaJS#7272 where it was suggested to remove these, so remove them.