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

Docs: Mixins with static properties #581

Closed
ackvf opened this issue May 14, 2020 · 4 comments
Closed

Docs: Mixins with static properties #581

ackvf opened this issue May 14, 2020 · 4 comments

Comments

@ackvf
Copy link

ackvf commented May 14, 2020

Forgive me for writing an issue instead of filing a Pull Request. I don't have enough experience with TS classes and Declaration merging to write a viable solution.

Page URL:
https://www.typescriptlang.org/docs/handbook/mixins.html
https://github.com/microsoft/TypeScript-Website/blob/v2/packages/handbook-v1/en/Mixins.md

Issue:

  • Inheritance via Mixins doesn't respect static properties on base class.
  • An interface which extends from class that defines static properties will ignore those s.p. since interface cannot define s.p. and in this case, the class is being treated as an interface, which without any warning exhibits this behavior.

    This treats the classes as interfaces

While the mixin function can be easily fixed, this doesn't hold true for the interface extending a class, which in turn renders the solution impractical when there are static properties.

Recommended Fix:

  • Mention these shortages on Mixin doc page.
  • Possible solution with implements? - class SmartObject implements Disposable, Activatable {}

And I have one follow-up question, which I would be happy to file an issue in TS repo if you deem it worthy: Would it be possible and does it make sense to explicitly warn when class that is being treated as interface foregoes its static properties?

@orta
Copy link
Contributor

orta commented May 14, 2020

Yeah, I think the mixin page probably needs a list of caveats at the end and a few more non-trivial examples. There are a few people who have a lot of experience with using mixins in TS, and maybe I can round them up to try get that all documented

The static properties issue sounds like a bug, and I couldn't find an issue on it (maybe I missed it, give it a look before making something with a sample )

@ackvf
Copy link
Author

ackvf commented May 14, 2020

Actually from what I read when trying to make the inheritance work in various ways, interfaces don't support static properties and when class is being treated as an interface which is the case in the example in mixins doc page, the static properties are ignored.

It is indeed an unexpected behavior without prior knowledge and can cause hours of debugging.

I would still like to get notified in the IDE about this though, because this is rather subtle feature.

@basarat
Copy link
Contributor

basarat commented Jul 3, 2020

Because it only copies members from prototype it only copies methods.

It also misses initialized properties:

import { applyMixins } from "./applyMixins";

class Disposable {
  isDisposed: boolean = false;
  dispose() {
    this.isDisposed = true;
  }
}

class Activatable {
  isActive: boolean = false;
  activate() {
    this.isActive = true;
  }
  deactivate() {
    this.isActive = false;
  }
}

class SmartObject {
  // ...
}

interface SmartObject extends Disposable, Activatable { }
applyMixins(SmartObject, [Disposable, Activatable]);

console.log(new SmartObject().isActive); // `undefined`. Expected `false`

And would also miss anything initialised in the constructors.

Also strict issue: #682

Thoughts

I feel like the caveat should be "don't use mixins by prototype mutation" 😂 🌹

@orta
Copy link
Contributor

orta commented Jul 16, 2020

Alright, the mixins docs got a rewrite in #719 👍

@orta orta closed this as completed Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants