-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Missing property declaration in abstract class implementing interfaces #4670
Comments
+1 for this use case. I was surprised it wasn't part of the original spec for abstract classes. Our use case is when making types for modules (such as sequelize) that use the definition object pattern to create objects from. With abstract classes + decorators we can define our types and our implementation details in one place, while creating the definition object for sequelize: abstract class Model<T> implements Sequelize.Instance<T> {}
@model<GroupInstance>()
abstract class GroupInstance extends Model<GroupInstance> {
// Static methods
@classMethod
public static joinedByUser(): string {
return 'test';
}
// Table attributes
@attribute({
type: Sequelize.STRING,
allowNull: false,
validations: {
notNull: true,
notEmpty: true
}
})
public name: string;
@attribute({
type: Sequelize.BOOLEAN
})
public school: boolean;
// Instance methods
@instanceMethod
public hello(): string {
return `hi there from ${this.name}`
}
// Validations
@validation
private isValid() {}
} But because we cannot extend the Instance interface without implementing it's members in the abstract class, we must use intersection types (not so pretty): abstract class GroupInstance extends Model<GroupInstance> {
...
}
type GroupI = GroupInstance & Sequelize.Instance<GroupInstance>; |
Although, the convenience of not writing the declaration would be nice, the possible confusion/complexity arising from this change would not warrant it. by examine the declaration, it is not clear which members appear on the type, is it all properties, methods, or properties with call signatures; would they be considered abstract? optional? |
@mhegazy I used to think that abstract classes are just like interfaces but with some members implemented. I know that TypeScript very often works non-intuitively different from other languages, but that is the difference in this specific case comparing to languages that don't have such problems with abstract classes? If you describe the problem with a concrete example I will appreciate this very much. |
For a motivating example: interface Clock {
enable(): void; // Specific to each implementer
disable(): void; // Specific to each implementer
setEnabled(value: boolean): void; // Common across implementers. Calls one of the above.
}
abstract class BaseClock implements Clock { // Complains that BaseClock doesn't implement all members of Clock. Of course it doesn't! It's abstract!
// Common implementation for all derived classes
setEnabled(value: boolean): void {
if (value) {
this.enable(); // If it didn't implement Clock, it couldn't call this anyway.
}
else {
this.disable();
}
}
} Of course we can keep declaring setEnabled in each derived class as an instance property And you can't work around this by adding stubs A workaround that continues to use ABCs is to implement everything in the ABC and duplicate everything that needs to be implemented in derived classes as abstract protected methods, which is ugly as hell. interface Clock {
enable(): void;
disable(): void;
setEnabled(value: boolean): void;
}
abstract class BaseClock implements Clock {
enable(): void {
this._enable();
}
disable(): void {
this._disable();
}
setEnabled(value: boolean): void {
if (value) {
this._enable();
}
else {
this._disable();
}
}
protected abstract _enable(): void;
protected abstract _disable(): void;
}
class DerivedClock1 extends BaseClock {
protected _enable() {
console.log("enable");
}
protected _disable() {
console.log("enable");
}
} |
Just ran into this, was very surprised by the behaviour. Why an already abstract class needs to redefine interface methods as abstract is beyond me. |
I agree that this behavior is very unexpected. |
yes, this does not make sense |
quite surprising behavior for me as well - don't think that it would introduce any risks, at least for scenarios that @mhegazy has mentioned. Actually, it could be much more easier for the code reader to dissect all the different methods and understand which particular behavior aspects where introduced either by interface or abstract class. And from the maintenance perspective, as @Arnavion has already mentioned here, it would be an additional work required to be done each time you'll change the interface - it would require additional changes in the abstract class which, in fact, should just introduce its behavior aspects which it is responsible for and, thus, it should not be touched each time the interface has changed. |
@Arnavion what worked for me and is a little bit less ugly was to only declare the interface implementation on the concrete class. interface Clock {
enable(): void;
disable(): void;
setEnabled(value: boolean): void;
}
abstract class BaseClock {
setEnabled(value: boolean): void {
// do something
}
}
class DerivedClock1 extends BaseClock implements Clock {
protected enable() {
console.log("enable");
}
protected disable() {
console.log("enable");
}
} Still feels a bit weird |
@piettes Your suggestion will not typecheck. |
Oh you are right; it only "works" if the abstract class doesn't call functions from the interface. |
Just small question, because this is for me probably most annoying feature of TypeScript - how much of you are using TS with PHP, thus you have a "compact" abstracts with the "heavy" ones on TS side? I have one general usage patter: interface IFoo {
enable(enable: boolean): IFoo;
proprietary(): IFoo;
}
abstract class AbstractFoo implements IFoo {
// ...code here
}
class FooBar extends AbstractFoo { // missing implements as it's enough to refer to abstract impl.
// ... cool stuff here
} I would be really happy too to not be forced to reuse all methods from interfaces in abstracts. |
I agree that this behavior is a bit unexpected. As an alternative, I've been getting around this using interface merging. The code below results in a compile time error:
I've been avoiding the compile time error by using something like this instead:
|
+1 |
1 similar comment
+1 |
Abstract class should not be required to implement all the properties/methods of the interfaces it implements. So the following should be legal:
But currently it gives:
This issue is related to proposal #3578.
The text was updated successfully, but these errors were encountered: