Upgrade to TypeScript v4.9.4 and fix all the errors

[kring]

Fixes #5219

The approach I used for the infamous TS2611 error is pretty simple, but I think it should work well. Instead of overriding trait properties like this:

get name() {
  if (super.name === "awesome") {
    return "so awesome";
  } else {
    return super.name;
  }
}

We instead declare a special Override method:

protected nameOverride(traitValue: string | undefined) {
  if (traitValue === "awesome") {
    return "so awesome";
  } else {
    return traitValue;
  }
}

If such a method exists, {traitName}Override, it is called and passed the trait value as computed from the strata. Whatever value it returns becomes the value of the {traitName} property.

This is implemented with a simple change to addModelStrataView. See here.

Derived classes can override this method, and can call super.nameOverride(traitValue) to call the base implementation. If we use this everywhere instead of overriding properties directly and calling the base with super.propertyName, then we can get rid of superGet, which is a nice side benefit. But in this PR, for ease of review (and perhaps a little bit of laziness), I didn't get rid of all property overriding, only the property overriding that typescript complained about. So we're not quite able to get rid of superGet just yet.

There are some (hopefully small) downsides to this new approach in terms of efficiency:

• Previously, an overridden property could avoid computing the strata value at all, simply by never calling super.propertyName. But now the strata value is always computed first, and then the class is given an opportunity to override it. If this ever matters (I doubt it), we can pass a function to compute the strata value to the {propertyName}Override method, rather than computing and passing the value itself.
• The work done by the Override method is not memoized separately, whereas the overriding property getter approach previously was, because it was declared as a computed property. So there might be some case where this causes a lot more work to be done. If so, it should be easy to fix using one of the techniques here: https://mobx.js.org/computeds-with-args.html. Or, if the strata value isn't needed, then simply delegate to a private computed property.

There were a handful of errors not realted to TS2611, and I fixed most of those by cherry-picking or copying changes from the ts4-interfaces branch.

[kring]

Hmm CI doesn't like this, but it's working well locally both in terriajs alone and in TerriaMap. Will investigate more tomorrow.

[kring]

I think this is in good shape now.

There were a couple of TS2611 errors that I thought were attempts to override trait properties, and so I added the new Override method. Specifically selectableDimensions and viewingControls. But these properties aren't actually traits, so the Override methods weren't called. It took me awhile to figure out why the compiler was generating TS2611 for these, and I'm still a little bit confused. But I eventually noticed that in the types that were having problems with these properties, CatalogMemberMixin was being mixed into them twice. It was being mixed in explicitly, and then also mixed in via TableMixin. Once I fixed that, the TS2611 errors went away.

[kring]

super.url no longer returns the non-overridden trait value; it invokes urlOverride and returns the exact same thing as this.url. So we need to go directly to the trait to get its value. IMO this should be refactored so that the url trait can be the normal URL, and a separate property provides the derived data request URL. But I decided not to get involved.

[steve9164]

This line probably needs a code comment

[kring]

As mentioned here, this is required to fix compatibility between the latest version of mobx-react that we can easily use, and TS 4.8+. As I understand it, TS still type checks libraries we actually use, this flag just makes it skip type-checking everything. So despite the scary name, I don't think this is a bad flag to use.

[na9da]

Great work @kring! I'll test it out on the esbuild branch sometime.

A quick thing I noticed about the new override mechanism is the use of protected qualifier, like in:

protected nameOverride(traitValue: string | undefined) {
  if (traitValue === "awesome") {
    return "so awesome";
  } else {
    return traitValue;
  }
}

One of our next goals is to package terriajs into a proper library that exports the built code and typescript type definitions (so that dependent applications/plugins need not build the whole of terriajs library when they want to consume it). This requires building the code with declaration set to true in tsconfig.json.

But when we do that typescript throws error TS4094 refusing to emit types for Mixins with protected or private members. This is discussed in more detail in this issue #5866. There are a few solutions for private members (like using the new JS # syntax for private) but none for protected members.

So, I think, we will need to strip the protected qualifiers turning all the protected members like forceLoadMapItems to public. I think this is an ok solution. But the same thing also affects the above bit of code. We should probably make it public and make it a rule to not call *override() methods directly.

[steve9164]

I like this approach.

To consider another, I was wondering if these override functions could be placed in automatic strata. The disadvantages of this would be that maintaining strata ordering between different mixins' automatic strata would be very important, and that is quite hard at the moment.

[steve9164]

This line probably needs a code comment

[steve9164]

Should the is X type here be ImageryParts?

Suggested change

	export function isImagery(mapItem: MapItem): mapItem is AbstractPrimitive {
	export function isImagery(mapItem: MapItem): mapItem is ImageryParts {

[kring]

I copied this from the ts4-interfaces branch, so I'm not actually sure. It sounds right to me, though.

[steve9164]

Extra questions/comments:

• Is there type checking on adding Override functions?
• If protected is to be removed from e.g. nameOverride, we may want to obscure these overrides more so that intellisense won't encourage people to try to use them.
  E.g. maybe it would be possible to put them all into an object on each (this changes proto, etc. but also might give us type checking possibilities?):

_traitOverrides: {
  name = (traitValue: string) => traitValue ?? "default name"
}

[kring]

Is there type checking on adding Override functions?

No, unfortunately. I couldn't see a straightforward way to add it.

If protected is to be removed from e.g. nameOverride, we may want to obscure these overrides more so that intellisense won't encourage people to try to use them.

Agreed!