Upgrade terriajs to typescript 4.x

[na9da]

Currently when using typescript 4.x with terriajs, we get a bunch of error TS2611. This ticket is to find a solution for the error and upgrade TS to the latest 4.x version.

[nf-s]

I want logical nullish assignment! 💥

[zoran995]

Thought that https://github.com/microsoft/TypeScript/pull/41994 might solve this issue, but just got the same error using it (maybe someone else wants to test it)

[na9da]

Thanks @zoran995 for following it up with the TS team.

I was thinking if we can find a workaround for this particular error.

When building terriajs with TS4.x we have around 256 errors out of which 223 are TS2611 and out of them 132 happens in LoadableStratum definitions and 91 in other models and mixins.

I was looking at the particular case of LoadableStratum, here is a simplified example of what is happening in LoadableStratum.

export type Constructor<T> = new (...args: any[]) => T;
export type PropertiesOf<T> = { [K in keyof T]: T[K] };

// A class factory for generating LoadableStratum
export function LoadableStratum<T extends Constructor<ModelTraits>>(
  Traits: T
): Constructor<PropertiesOf<InstanceType<T>>> {
  type Traits = InstanceType<T>;

  abstract class Loadable {
     // ... code that dynamically defines getters/setters for traits
  }

  return Loadable as any;
}

class ModelTraits {}

class FooTraits extends ModelTraits {
   get foo() { return 0; }
}

class FooLoadableStratum extends LoadableStratum(FooTraits) {
    // TS2611 error here even though foo is a getter in FooTraits
    get foo() {
         return 1;
    }
}

Typescript play link

Even though foo is defined as a getter in FooTraits, TS doesn't allow me to extend it as a getter in the loadable stratum. What I think is happening is that PropertiesOf<T> in the return type of LoadableStratum erases the fact that foo is a getter and treats it as a property. And TS2611 does not allow the derived class FooLoadableStratum to implement a getter for foo.

In LoadableStratum you can see that the factory function doesn't do much other than defining getters/setters for the traits and returning an abstract class which we inherit from.

Instead of using inheritance to achieve this we could use a decorator function to enhance our Loadable stratum definition. Here is an example implementation that I think might work although I haven't fully tested it.

export type Constructor<T> = new (...args: any[]) => T;
export type PropertiesOf<T> = { [K in keyof T]: T[K] };

class ModelTraits {}

class FooTraits extends ModelTraits {
  get foo() { return 2; }
  get bar() { return 3; }
}

function LoadableStratum<T extends Constructor<ModelTraits>>(Traits: T) {
  return function<C extends Constructor<Partial<PropertiesOf<InstanceType<T>>>>>(Class: C) {
     // Here we can defineProperty on Class.prototype to add traits not implemented by the stratum itsel
     return Class;
  }
}
 
@LoadableStratum(FooTraits)
class LoadableFooStratum {
   get foo() {
     return 23;
   }
}

const fooStratum = new LoadableFooStratum();
// because bar is not implemented by LoadableFooStratum and decorators in TS cannot change the type of its input class 
// we get an error for bar. But this is alright for our usage of loadable stratum.
console.log(fooStratum.foo, fooStratum.bar);

Typescript play link

So we might be able to solve the TS2611 errors for LoadableStratum by getting rid of inheritance and using decorators. Fixing the error in mixins and models might be harder though and will need another solution.

[nf-s]

Ideas

• Maybe use object (not class) based LoadableStratum
• Maybe ban overriding traits in Models (for example GeoJsonCatalogItem)
  • This means all trait values will need to go into strata

...

[na9da]

Why TS2611

class Base {
  foo = 1;
}

class Bar extends Base {
  get foo() { return 42; }
  set foo(a) { }
}

const bar = new Bar();
console.log(bar.foo);
// Expected output: 42
// Actual output: 1

TS is trying to stop shooting ourselves on the foot by refusing to compile the above JS code that can lead to silent bugs. The error was introduced in TS 4.something

What changed in TS4.8

From version 4.8, TS suppresses this error when the overridden property is defined as abstract in the base class or comes from an interface definition.

The good news is, this fixes a ton of errors for us when overriding traits in LoadableStratum definitions.

However we have around 30 remaining errors when overriding traits in Mixins (or CatalogItems).

Remaining TS2611 errors:

     17 cacheDuration
      1 chartItems
      1 description
      1 disableZoomTo
      2 name
      1 resultsAreReferences
      3 selectableDimensions
      3 shortReport
      1 url
      1 viewingControls

The error on cacheDuration trait can probably be fixed by setting a default trait value instead of overriding it in a mixin or catalog item. But there are still a few useful overrides like the name which we might want to retain.

Why does it fix LoadableStratum errors but not mixin errors?

The return type of LoadableStratum() is an explicit interface type that defines traits as properties of thes stratum. Since 4.8, TS exempts properties defined in an interface or properties of classes that are abstract from this error.

On the contrary, our mixins do not have an explicit return type and so TS treats it as a regular class for which there is no exemption.

Possible fix

• Explicitly type the return values of a Mixin using an interface like in this official mixin example - https://github.com/microsoft/TypeScript/blob/6addf091dfadbba191c7bbfdc9eda2410d24b260/tests/baselines/reference/accessorsOverrideProperty9.js#L21-L29. Notice how they create an interface ApiItemContainerMixin that defines the shape of the mixin class and uses it in the return type.

• Good news: this also fixes some of the mixin errors that we get when emitting type declarations - (point 3 - https://github.com/TerriaJS/terriajs/issues/5866#issuecomment-933964504).

• Bad news: Because interfaces do not have the notion of protected members we have to either convert some of the methods like forceLoadMapItems to public methods or resort to some hacky and unsafe type casting. (point 4 - https://github.com/TerriaJS/terriajs/issues/5866#issuecomment-933964504). I prefer making the protected members public as it is a much simpler solution. Altogether, there are around 10 different protected methods in our mixins.

• Bad news: Defining an explicit return type becomes a bit tedious for some of our larger mixins and mixins that implicitly include other mixins.

• Good news: we might be able to get away with explicitly typing only a few main mixins.

TLDR

Since TS4.8, properties defined as part of an interface are exempt from TS2611 error which permits these properties to be overridden as getters/setters. This fixed a ton of errors in LoadableStratum but we still have 30 errors in mixins. Fixing these errors requires explicitly typing the return value of mixin functions as an interface (see official example - https://github.com/microsoft/TypeScript/blob/6addf091dfadbba191c7bbfdc9eda2410d24b260/tests/baselines/reference/accessorsOverrideProperty9.js#L21-L29). This is tricky to do correctly when using protected members in a mixin, so we have to convert some of the protected members like forceLoadMapItems and forceLoadMetadata to public (there are 10 of them). After that we can explicitly type the return value of our mixin functions (although it'll be a bit tedious for the larger mixins). Once all (or some?) of our Mixins are explicitly typed, it should be possible to override traits inside any mixin class or any catalog item class.

[na9da]

I had another go at this.

I tried two approaches:

Explicitly type all Mixin return values

I started off with this approach but soon found issues which made me think that it is a no-go option. As explained under Fix#1 in the above comment, we explicitly type all Mixin returns by defining an interface: https://github.com/TerriaJS/terriajs/blob/356b05377a1a5c29a9cb24cf5c7567a89f6f76fc/lib/ModelMixins/TableMixin.ts#L66-L93

Issues:

1. The typescript handbook says this about mixins.

    // Mixins may not declare private/protected properties
    // however, you can use ES2020 private fields

I think explicitly typing the Mixin return values is dangerous because it can hide private and protected members. When derived classes accidentally override these members, there will be no warning and can result in hard to debug bugs. Here is an illustration of this bug.

2. I saw strange side effects when specifying explicit types for Mixins. For eg, if we have a type like: let model: ArcGisMapServerCatalogItem | ArcGisMapServerCatalogGroup; and later we do model.setTrait(CommonStrata.user, "url", "something), TS is unable to resolve the correct setTrait() to use for the call. This works ok when I am not explicitly typing the mixin return. I am not sure why it happens.You can see this happen in ArcGisMapServerCatalogGroup.

I think by not fully understanding the repercussions, we will work ourselves into a corner if we go with this approach

Ignore unsolvable TS-2611 errors

In this second approach (available here) I did the following:

1. 17 out of the remaining 29 TS2611 errors are from overriding cacheDuration trait in catalog items and mixins. I have removed these overrides, which means it'll default to no caching when using proxy. Let's discuss this change, but I feel enabling caching by default was probably not a good idea.
2. Change selctableDimensions and viewingControls getters to methods. I decided not to ts-ignore the errors here because these methods are likely gonna be used a lot more so it is best if we fix them now.
3. ts-ignore all the remaining TS2611 errors.

We could gradually move some of these overrides to a loadable stratum where it makes sense or provide some helper mechanism to define it from within the class.

I feel this is the best approach we have now because we won't be committing to a change we don't understand completely. Further, if we want to get rid of superGet we have to stop overriding @computed values everywhere. This means we have to learn to keep mixins really simple, by not writing overridable behaviours as computed, not writing protected members and only using JS #syntax for private members.

Note that TS 4.8 breaks some of our dependencies so to get it to fully compile without errors we have to enable skipLibCheck in tsconfig.json. The remaining errors on tinymce looks fixable when the release the next version. mobx-react fixed the error in their latest version however we are stuck on mobx-react 6.x which probably wont get another release, so we have to think of publishing terriajs-mobx-react as a stop gap.

This will also break other maps which we will have to test one by one.

[AnaBelgun]

Update Nov 22:

• decision needed for errors handling

[AnaBelgun]

Connected to https://github.com/terriajs/terriajs/issues/5866

[steve9164]

Can we avoid publishing a new mobx-react by overriding some types in a .d.ts file? (Like we do for some Cesium types)

[steve9164]

Notes from out meeting yesterday on changing property access to functions:

• What does this mean for computed? Can we make the methods computed?
• If not, methods can be defined by returning the value of one computed
• Still use a finaliser so that UI continues to get values by property access
• Does this pose a problem for type checking?

Experimentation:

const props = {a: 'a', e: 'e', i: 'i', o: 'o', u: 'u'};


type StringFunction = () => string;

type PropsType<T> = {
  [K in keyof T]: StringFunction
}

type Constructor<T> = new () => T;

function makeClass<T extends {}>(props: T) {
  class BaseKlass {
    b: number;
    constructor() {
      this.b = 15
    }
  }

  Object.keys(props).forEach(k => {
    (BaseKlass as any).prototype[k] = function() {
      return "" + this.b;
    }
  });
  return BaseKlass as unknown as Constructor<PropsType<T>>;
}

const C = makeClass(props);

const instance = new C();

instance.a()

// Error:
// Class 'PropsType<PropsType<{ a: string; e: string; i: string; o: string; u: string; }>>'
//  defines instance member property 'a', but extended class 'D' defines it as instance 
//  member function.ts(2425)
class D extends C {
  a() {

    // Error:
    // Only public and protected methods of the base class are accessible via the 
    //  'super' keyword.ts(2340)
    return super.a() + ' metres of string';
  }
}

• TS 2425: Allow mapped type to declare functions · Issue #48125 · microsoft/TypeScript · GitHub

Even using:

interface IC extends PropsType<typeof props> {};
const C = makeClass(props) as Constructor<IC>;

the problem still exists

[steve9164]

Override system using endomorphisms - "mapping functions" (traitValue: T) => T:

• Have an override system where mixins can define "mapping functions", which BaseModel (or Model) will call with the final trait value when a trait is accessed e.g. item.name.
• Mixins must be able to access the output of other mixins' overrides further up the chain
• One implementation of this is to allow mixins to define e.g. nameOverride(traitValue: string): string
  • Hard to type-check
• Another implementation is outlined below:

const props = {a: 2, e: 'e', i: 'i', o: 'o', u: 'u'};

type StringTransformer = (val: string) => string;

type TypeTransformer<T> = (val: T) => T;

type WithOverrides<T> = {[K in keyof T]: T[K]} & {
  _traitOverrides: {
    [K in keyof T]?: TypeTransformer<T[K]>
  }
}

type Constructor<T> = new () => T;

function makeClass<T extends {}>(props: T) {
  class BaseKlass {
    b: number;
    _traitOverrides = {};
    constructor() {
      this.b = 15
    }
  }

  Object.keys(props).forEach(k => {
    (BaseKlass as any).prototype[k] = function() {
      return (props as any)[k];
    }
  });
  return BaseKlass as unknown as Constructor<WithOverrides<T>>;
}

const C = makeClass(props);

const instance = new C();

// A mixin would do something like:
class D extends C {
  constructor() {
    super();
    const superOverrides = this._traitOverrides;
    this._traitOverrides = {
      ...superOverrides, 
      a: (val: number) => superOverrides.a?.(val) ?? val * 100 + this.b
    };
  }

}

Disadvantages of _traitOverrides object approach:

• Function/closure per instance, instead of per mixin/class
• Re-implements inheritance
• Boilerplate in each mixin
  • Could be alleviated by hiding inside a function:

// constructor() {
// ...
this._overrideTraits((superOverrides) => ({
  a: (traitValue: number) =>
    superOverrides.a?.(value) ?? traitValue * 5 + this.b,
}));

[na9da]

Update: So far we have two solutions [1, 2] for upgrading to TS4.x which we describe below. Any solution for TS2611 should also be compatible with mobx6. Also note that we are trying to get rid of the use of superGet when overriding mobx @computed and @action values so that we can simplify terriajs build process.

On a closer analysis, there are two kinds of TS2611 errors:

1. Error when overriding accessors originating in Mixins like selectableDimensions or viewingControls
2. Error when overriding traits like name, nameInCatalog or cacheDuration

The basic trigger for both errors is overriding accessors that has multiple base declarations.

Type 1 errors

These happen when some Mixin enters the mixin hierarchy more than once. For eg, CatalogMemberMixin is included twice in GeoJsonMixin, directly and via TableMixin.

These errors can be fixed by removing duplicate Mixins from the hierarchy.

Type 2 errors

Fundamentally, these are also triggered when overriding some trait which has more than one base declaration. Eg, KmlCatalogItem has 2 declarations for cacheDuration. One from the return type of CreateModel(KmlCatalogItemTraits) call and another from the base type of UrlMixin.

Unlike type 1 error, there is no easy way to remove the multiple base declarations in this case.

We have implemented 2 solutions to fix TS2611 errors when overriding traits.

Solution 1

Define a hook at each class level that returns the trait overrides.

Eg from GeoJsonMixin: https://github.com/TerriaJS/terriajs/blob/38b61aaaf5bad4369a5e9237d502533b8cc5ba7e/lib/ModelMixins/GeojsonMixin.ts#L337-L355

Any mixin or catalog item can define the hook _newTraitOverrides and return an object containing trait override functions.

Pros:

• Type safe by default
• Future proof. Can't say what breaking changes will be introduced in the future, but this relies on a simple mechanism which shouldn't break

Cons:

• Introduces a new pattern for overriding traits
• There are trait overrides that don't trigger TS2611 errors when overriding (eg). This might result in inconsistency in how we override traits.
• In some future TS universe, if mapped types get support for definining get / set accessors, then we will want to change it back to the current implementation.

Thanks @kring for working out this solution.

Solution 2

Typescript 4.9 introduced an escape hatch for TS2611 errors. When an accessor has multiple base declarations, then the error is exempted if any 1 of those base declaration comes from an interface definition. That means we can fix the errors by declaring an interface with the same name as the trait classes.

Eg for UrlTraits: https://github.com/TerriaJS/terriajs/blob/1a2295507cd064e7d14ba9f9c31d9bfce4bb58f8/lib/Traits/TraitsClasses/UrlTraits.ts#L28-L34

Note that for this solution to work, both the class and the interface must be defined in the same module. So if an end developer wants to override a new trait that is not part of the interface definition in terriajs, then they'll have to do something like:

class MyGeoJsonCatalogItemTraits extends GeoJsonCatalogItemTraits {}
interface MyGeoJsonCatalogItemTraits {
   description?: MyGeoJsonCatalogItemTraits["description"]
}

class MyGeoJsonCatalogItem extends GeoJsonMixin(MyGeoJsonCatalogItemTraits) {
   get cacheDuration() { return super.cacheDuration ?? "0d" };
}

Pros:

• Incremental fix, no change to how we currently override traits.
• In some future TS universe, if mapped types get support for definining get / set accessors, then we can simply remove the trait interfaces.

Cons:

• Interfaces can define non-existent traits. So type safety is reliant on using a convention where we define the trait type by referencing it from the class, eg: cacheDuration?: UrlTraits["cacheDuration"]

Note that both implementations now use mobx6 and can use the new @override decorator for overriding base values without using superGet.

[AnaBelgun]

Update 1 Feb 2023:

• @na9da to make two PRs for those two options / branches to make commenting easier and link to this PR

[AnaBelgun]

Update 15 Feb 23:

• PR in progress https://github.com/TerriaJS/terriajs/pull/6687

[AnaBelgun]

Update 26 Apr:

• made final changes to be followed by @steve9164 review
• Step 1 - release into SaaS from a different branch
• Step 2 - release post-SaaS update, but after DTV Apr release is finalised

[na9da]

Done.

Please refer to upgrade guide for info on upgrading.