tannerntannern / ts-mixer

A small TypeScript library that provides tolerable Mixin functionality.
MIT License
379 stars 27 forks source link

ts4.2 abstract constructors #32

Closed tonygjerry closed 3 years ago

tonygjerry commented 3 years ago

Have you seen https://devblogs.microsoft.com/typescript/announcing-typescript-4-2-beta/#abstract-construct-signatures? Will caveat 1 be solvable after that? https://github.com/tannerntannern/ts-mixer#caveats

tannerntannern commented 3 years ago

Thanks for bringing this to my attention @tonygjerry! And yes, I believe you're correct about this resolving the abstract class caveat for ts-mixer. That said, I'm not sure I want to implement this right away because it would force people to upgrade to TS 4.2 at a minimum, even though some folks may still be as far back as 3.8.

I will leave this issue open as a reminder to myself, but I don't plan on addressing it at least until 4.2 is out of beta. Glad to know that we'll be able to remove one of the caveats in the future though!

chrisdostert commented 3 years ago

@tannerntannern thanks for the awesome library! Any update on this now that 4.2 is out of beta and 4.3 is out?

tannerntannern commented 3 years ago

Hi @chrisdostert, I'll try to cut out some time for this in the coming days. It should be a relatively simple change. However, I think it will still need to be major release because of the TS >=4.2 requirement.

tannerntannern commented 3 years ago

@tonygjerry, @chrisdostert, v6.0.0-beta.0 has been released, which adds proper abstract class support. Since you both expressed interest in this feature, please give it a try and let me know if it behaves as you expect. If I don't hear back after a few weeks I'll promote it to a full release.

tonygjerry commented 3 years ago

Cool, I'm trying it out and it seems to work, seems like abstract classes can be both parents and children to mixins with 6.0 beta. I'll continue to monitor and let you know.

btw, I don't see any pr or commits in github for this new release?

tannerntannern commented 3 years ago

btw, I don't see any pr or commits in github for this new release?

Since I'm the only maintainer, I don't typically create PRs for releases. All releases are triggered from tags. For beta releases I often create the tags from feature branches, and for full releases I merge to master in addition to creating a tag. If you're looking to see what changed since the last release, you can compare tags: https://github.com/tannerntannern/ts-mixer/compare/v5.4.1...v6.0.0-beta.0

tonygjerry commented 3 years ago

I think I found an issue - this update seems to break the type inference of generics set by the mixin parent

export interface Example {
  key: string;
}
class A<T extends unknown = Example> {
  protected data: T;
  constructor(data: T) {
    this.data = data;
  }
}
class B {}

export class Child extends Mixin(A, B) {
  public foo() {
    // type interference of bar is any
    const bar = this.data;
  }
}

Before, it correctly inferred the type to use the generic's default type. not sure if that is an actually supported feature or not, as its not mentioned in the generics workaround (https://github.com/tannerntannern/ts-mixer#mixing-generic-classes)

tannerntannern commented 3 years ago

I don't believe this was ever supported without using the generics workaround. Your example reworked would look more like this:

import { mix } from 'ts-mixer';

export interface Example {
  key: string;
}

class A<T extends unknown = Example> {
  protected data: T;
  constructor(data: T) {
    this.data = data;
  }
}

class B {}

export interface Child extends A, B {}
@mix(A, B)
export class Child {
  public foo() {
    // type interference of bar is Example
    const bar = this.data;
  }
}

Playground link

(Note, I haven't had a chance to test this locally. I don't think the TS playground pulls beta releases automatically)

tannerntannern commented 3 years ago

6.0.0 has been published which adds support for abstract constructors