Fix TS4094 Declaration emit Error for mixins

[t83714]

See:

• #5854
• And PR: https://github.com/TerriaJS/terriajs/pull/5863

Out of 133 type errors we need to solve,

• error TS4094: Property 'xxxx' of exported class expression may not be private or protected.
  • Might related to this. i.e. exported anonymous classes can't have private or protected members, because there's no way to represent that in a .d.ts file.

This ticket is about solving the errors (from the google docs link below) that are related to issue above.

https://docs.google.com/spreadsheets/d/12lDW0jfA-2x6A4LE9J9BSXXaWNpClk1Ypm4_E8kB0bo

[na9da]

There are probably 2 ways to fix this error:

1. Explicitly specify the return types of all mixin functions. This can be really hard to do for some of our larger Mixin classes.
2. Replace private and protected members with the _ prefixed public members. This could be the easiest fix.

Other possible approaches are discussed here - https://github.com/microsoft/TypeScript/issues/17744#issuecomment-558990381

[t83714]

Comment from @na9da for his investigation result & possible options:

So, I took a closer look at this issue for fixing TS4094 https://github.com/TerriaJS/terriajs/issues/5866 . The error basically says TS cannot emit type declarations for mixin classes with private or protected members. When compiling with tsc with declaration: true , out of aprox 130 errors, 80 of them are TS4094. This happens to a lot of people, they start off with declaration: false and then when they turn it on, they have this error which is tricky to work around. This thread lists some possible options which I had a look at https://github.com/microsoft/TypeScript/issues/17744#issuecomment-558990381 . I'll summarize what I think are our options (I think we need to use a mix of them). Use private/protected members sparingly in Mixin classes For private mixin state, use #name es2020 private variable syntax. TS doesn't throw error for them. However we cannot use decorators on them so for an observable private state we have to do something like #name = observable("foo") Also TS 3.x doesn't support #privateMethod() so we can't use this right now for private methods. 3. If you want to use private methods, you have to declare an interface and explicitly type the return value of the Mixin function using the interface so that TS knows exactly what type to emit. 4. If you want to use protected members, it becomes a bit more difficult and is also type-safety wise unsound. Because interfaces cannot qualify members as private/protected we have to instead use a template class. eg declare class FooMixinTemplate { protected foo(): string } and use that to type the return value of the Mixin. Also because FooMixin derives from the Base parameter and not FooMixinTemplate we have to force the cast from FooMixin to FooMixinTemplate . As a result of the forced casting, there is no guarantee that FooMixin correctly implements FooMixinTemplate. There is no other known workaround for protected Mixin members. So I think we should use it sparingly only for some things like forceLoadMapItems. In some far future where TS supports this use case, we can just remove the template class and everything will work. 5. One last option is to use _name and make the member public. This will be a breaking change and will pollute type suggestions.

[t83714]

I think we probably can have a try the "Declare the return type explicitly" approach (might similar to the option 3 @na9da suggesting ). i.e.:

export function test (): new() => Object {
    return class {
        private privateMember () {
        }
    };
}

[t83714]

Sample code for two solutions (option 3 & 4) from @na9da

export type Constructor<T> = new (...args: any[]) => T;
​
class BaseModel {
  baseMethod() {
    return "baseMethod";
  }
}
​
declare abstract class SimpleMixinTemplate {
  abstract baz(): string;
  simple(): string;
}
​
export function SimpleMixin<T extends Constructor<BaseModel>>(
  Base: T
): T & Constructor<SimpleMixinTemplate> {
  abstract class SimpleMixin extends Base {
    abstract baz(): string;
    private privateState = "privateState";
    private privateMethod() {
      return this.privateState;
    }
​
    simple() {
      return this.privateMethod();
    }
  }
​
  // Option 3: No casting required  here when not using `protected` members.  So TS will
  // report errors if our SimpleMixin implementation differs from the
  // advertised interface in SimpleMixinTemplate.
  return SimpleMixin;
}
​
class TestSimple extends SimpleMixin(BaseModel) {
  baz() {
    return "baz";
  }
}
​
function testSimple() {
  const test = new TestSimple();
  console.log(test.simple(), test.baz(), test.baseMethod());
}
​
declare abstract class ProtectedMixinTemplate {
  protected abstract bar(): string;
  foo(): string;
}
​
export function ProtectedMixin<T extends Constructor<BaseModel>>(
  Base: T
): T & Constructor<ProtectedMixinTemplate> {
  abstract class ProtectedMixin extends Base {
    protected abstract bar(): string;
    foo() {
      return this.bar();
    }
  }
​
  // Option 4: We need to force the cast. So TS won't throw any errors if the mixin implementation
  // deviates from the advertised interface in ProtectedMixinTemplate.
  return ProtectedMixin as unknown as T & Constructor<ProtectedMixinTemplate>;
}
​
class TestProtected extends ProtectedMixin(BaseModel) {
  bar() {
    return "bar";
  }
}
​
function testProtected() {
  const test = new TestProtected();
  console.log(test.foo(), test.baseMethod());
}
​
function test() {
  testSimple();
  testProtected();
}
​
test();

[t83714]

had a chat with @na9da so to make it clear:

• the only solution that we have & work is option 4 above.
• but it increases the complexity of the code We need to make a decision that whether we are happy with this solution. If we are not happy, we probably can park the ticket for now as we don't have a good enough solution.

[t83714]

Here is a playground example of option 4: Playground Link

[t83714]

There is also a symbol-based property hiding solution I think might help with an easier solution under certain circumstances : https://github.com/microsoft/TypeScript/issues/17744#issuecomment-431534647 @na9da suggested could be a supplementary solution to option 4. more reading on this solution: https://component.kitchen/blog/posts/hiding-internal-framework-methods-and-properties-from-web-component-apis

[t83714]

@na9da created a new branch that adopted a different approach to solve this: https://github.com/TerriaJS/terriajs/compare/emit-types

The main changes include:

• Replacing private someMember in mixins with _someMember That makes it public. In future mixins though, we can write private methods outside as loose JS functions so that they are not included in the mixin.
• Replaced protected someMember with just someMember This again makes it public. But I think it is a fair compromise, Also we can document that forceX methods shouldn't be invoked by end-developer. This will also be a breaking change because TS won't allow a public method to be overridden as a protected one. But it should be easy to fix in downstream repos.
• Converted a few .js files to .ts. I think it's a good & simple solution --- if everyone's happy with some of the compromises (for protected methods) made here, I think we should just go for it.