Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TS4094 Declaration emit Error for mixins #5866

Open
t83714 opened this issue Sep 29, 2021 · 8 comments
Open

Fix TS4094 Declaration emit Error for mixins #5866

t83714 opened this issue Sep 29, 2021 · 8 comments
Assignees
Labels

Comments

@t83714
Copy link
Member

t83714 commented Sep 29, 2021

See:

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

@t83714 t83714 changed the title Fix TS4094 Error for mixins Fix TS4094 Declaration emit Error for mixins Sep 29, 2021
@na9da
Copy link
Collaborator

na9da commented Oct 1, 2021

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 - microsoft/TypeScript#17744 (comment)

@t83714
Copy link
Member Author

t83714 commented Oct 5, 2021

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

So, I took a closer look at this issue for fixing TS4094 #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 microsoft/TypeScript#17744 (comment) . 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
Copy link
Member Author

t83714 commented Oct 5, 2021

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
Copy link
Member Author

t83714 commented Oct 5, 2021

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
Copy link
Member Author

t83714 commented Mar 11, 2022

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
Copy link
Member Author

t83714 commented Mar 12, 2022

Here is a playground example of option 4: Playground Link

@t83714
Copy link
Member Author

t83714 commented Mar 12, 2022

There is also a symbol-based property hiding solution I think might help with an easier solution under certain circumstances :
microsoft/TypeScript#17744 (comment)
@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
Copy link
Member Author

t83714 commented Mar 25, 2022

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants