Open Xample opened 6 years ago
Instead of requiring overriders to remember to call super
, you could do a design like so:
class A {
final exit() {
// do some important cleaning stuff
}
/** Override me to your heart's content. */
onExit() {}
}
class B extends A {
exit() { /* forget to call super() */ } // Errt, not allowed, no overriding 'exit'
onExit() { } // Write whatever you want, the important cleanup will be done in 'exit'
}
If we had #1534. Would that be sufficient for your use case?
“final” or “seal” (as in #1534) are not sufficient. I have no hand on who calls exit(). Imagine the angular 4’s “onDestroy” hook, if I want to make a baseComponent implementing the “onDestroy” and to extend this component, I must remember my parent’s to already have it’s onDestroy. The framework is calling the method, the interface is given by the framework, therefore I cannot rename it.
If you can't rename it, you presumably can't add the concrete
modifier to it either?
The framework is giving you the interface. You would add concrete
on the class implementation (not on the interface).
A workaround: Declare that onExit()
returns a "receipt" that proves it was run on the superclass, and make sure that the only (reasonable) way to get such a receipt is by calling super.onExit()
. In the following case, ExitReceipt
is a private static member of A
: so methods of A
can refer to the value A.Receipt
, but methods of B
and C
cannot. And the type of A.ExitReceipt
is an instance of an anonymous class with a private member, so you can't easily get a new one.
class A {
private static ExitReceipt = new (class {
private property: string = "";
})();
onExit(): typeof A.ExitReceipt {
// do some important cleaning stuff
return A.ExitReceipt;
}
}
class B extends A {
onExit() {
// do some before-cleanup stuff
const exitReceipt = super.onExit();
// do some after-cleanup stuff
return exitReceipt;
}
}
class C extends A {
// doesn't work since it doesn't return a receipt
onExit() {
}
}
Does that make sense?
Which other languages that support OOP have this construct?
I found the @CallSuper decorator in Java https://developer.android.com/reference/android/support/annotation/CallSuper.html
usage
@CallSuper
public abstract void onFocusLost();
But in this implementation (to my understanding), the @CallSuper decorator forces the method to call "super". In my case, any subclass should implicitly inherit this @CallSuper decorator. In their Java example, they ensure this behavior adding the abstract keyword.
@jcalz interesting hack but I don't have the hand on the interface, only the implementation
@Xample I find the construct interesting, in that it enhances the contract between classes of the inheritance tree. I don't recall such a construct in other OO programing languages.
I think the keyword semantic should refer to some idea of nesting, since each descending class in the inheritance tree has to call the upper super
method... Perhaps nested
or onion
. "mandatory" sounds like the implementation is mandatory (like "abstract") and "concrete" is weird in that the opposite of abstract would rather be "any defined method", which is orthogonal to this construct.
@sveinburne english not being my mother tongue, its hard for me to find a keyword which really sounds good. What I can think about is that the function should have a name referencing that it will always be called i.e. we cannot shadow this method. You want to call it "bright" ? or maybe "root" as you refer to a tree ?
I think your problem can be resolved with this:
class A {
// this is private then is not overridable
private onExit() {
// do some important cleaning stuff
// call overridable method empty in base class
this.OnExit();
}
OnExit() { } // or abstract
}
class B extends A {
OnExit() {
}
}
}
@dardino it's a solution if it's your own code and you do not rely on interfaces. "onExit" being part of an interface it cannot be private. The real life use of my proposal is mostly for hooks. Like we see in angular or [ionic] (https://ionicframework.com/docs/nightly/api/navigation/NavController/#lifecycle-events) Lifecycle events (but not restricted to).
let's take the following example:
export class BaseComponent implements OnDestroy {
ngOnDestroy() {
// clean up stuff
}
}
where OnDestroy is as follow:
export interface OnDestroy {
ngOnDestroy(): void;
}
now imagine I would extend this class
export class AClass extends BaseComponent {
ngOnDestroy() {
// problem if I do not call the super ngOnDestroy() function
}
}
A "mandatory" keyword would print an error if we forget to call the super method. (as it is already the case with constructors of class extending another one)
@Xample I had the same problem with trying to ensure that classes that extended a BaseEventService class called super.ngOnDestroy
if they override the ngOnDestroy
in their implementations. I couldn't find a way to enforce it with warnings/TypeScript constraints, but I can ensure that the functions in the Base Class get called regardless of how the sub classes are implemented
One way to do this, which works if the sub classes call ngOnDestroy
, ngOnDestroy(){ super.ngOnDestroy()}
, or if they don't implement ngOnDestroy
at all
In the Base Class
constructor() {
super();
let originalOnDestroy = this.ngOnDestroy;
this.ngOnDestroy = () => {
this.destroy();
originalOnDestroy.apply(this);
};
}
ngOnDestroy(): void {
// do nothing
}
private destroy() {
// clears all subscriptions etc
}
The keyword could be required
.
@lukescott like required in swift ? https://stackoverflow.com/questions/26923123/what-does-the-required-keyword-in-swift-mean
In swift it means the subclass needs to implement that method one (a little bit like ‘abstract’ keyword) not that he subclass’s method have to call the super one.
That would be a very nice feature in addition to TypeScript 2.2 mixin syntax. For example, I have a mixin that defines componentWillUnmount
method for React.Component which performs some kind of destruction finalization. Than I implement a class that extends this mixin and implements own componentWillUnmount
- it is very easy to miss super.componentWillUnmount
call that may lead to bugs that are hard to track.
We are working with Ionic/Angular, in our project there are a couple of base classes that implements standard functions centralised for all others inherited classes (pages/services/etc)
Using these frameworks is rather frequent to override methods (hooks) that let us control the lifecycle of components, pages, etc.
It is a real pain have to manually check that in each method override there is a proper calls to super().
Definitively I add my vote for this feature request, a simple keyword like required
or mustcall
to let the compiler emit an alert on the console could be enough.
@RyanCavanaugh:
Which other languages that support OOP have this construct?
For example, Java and C# does not suffer from this problem, because they require to explicitly use override
keyword on order to override methods.
public class Base
{
public virtual void A() => Console.WriteLine("Base.A");
public virtual void B() => Console.WriteLine("Base.B");
}
public class Concrete: Base
{
public override void A() { } //thanks override keyword developers are aware of the base.A() method
public void B() {} //throws an error or warning that Concrete.B hides Base.B
public new void B() => Console.WriteLine("Cocnrete.B") // keywork new explicitelly hides Base.B
}
Concrete concrete = new Concrete();
concrete.B(); //writes Concrete.B
Base base_ = concrete;
base.B(); //writes Base.B
However in typescript, it can very easily happed that a method is overridden unintentionally. Think of following inheritance
class Base {}
class Concrete1 extends Base
{
ngOnDestroy() { console.log("Concrete1.ngOnDestroy" }
}
this is proper inheritance. However, if at some point we would add ngOnDestroy to Base class, it would render all extended classes invalid, since they would override Base.onOnDestroy() automatically.
@Liero “I assume concrete extends Base” As you wrote, “override” only explicitly tell the compiler that the coder knows what he is doing. I have no problem of overriding a method even without the “override” keyword, my problem is more to ensure a super method to be called (like it is the case for a constructor). “override” is another interredting feature btw
Is this summary correct?
C++ | C# | TypeScript | Typescript proposed additions for non-breaking change | Opinionated perfect design? | |
---|---|---|---|---|---|
on class | final, virtual | sealed, abstract | abstract | final/sealed | enabled some sort of 'extendable' keyword on class |
defaults | unspecified (lol) | virtual class | 'virtual'(proto based) class | 'virtual' | sealed, adding virtual methods makes classes virtual, 'extendable' classes |
on method | final, virtual, override, 'abstract', no 'callbase' on virtual, final, override | sealed, virtual, override, new | abstract | final/sealed, call-super/callsuper/call_super | enabled keywords: virtual, abstract, override, final/sealed, callbase/callsuper even on non-virtual methods |
defaults | 'could-be-shadown', 'could-shadow', 'non-virtual' | 'could-be-shadown', 'could-not-shadow', 'could-virtual' | 'virtual' | 'virtual' | 'non-virtual' but 'could-shadow' with new and 'could-be-shadown' |
concrete
keyword imho is confusing in naming with virtual
which all methods in TypeScript (kind of) are. Let's call it callsuper
or required-super
and be done with it lol.
@Xample I feel like in the protected override method()
case, the compiler would not throw if super.method() was called in the child
example (using angular hook for demo purposes):
class Parent implements OnInit {
public ngOnInit(): void {
console.log('in parent ngOnInit');
}
}
class ChildA extends Parent {
public ngOnInit(): void { // should throw
console.log('in child ngOnInit');
}
}
class ChildB extends Parent {
public ngOnInit(): void { // should NOT throw
super.ngOnInit();
console.log('in child ngOnInit');
}
}
class ChildC extends Parent {
public override ngOnInit(): void { // should NOT throw
console.log('in child ngOnInit');
}
}
class ChildD extends Parent {
public override ngOnInit(): void { // should throw ?
super.ngOnInit();
console.log('in child ngOnInit');
}
}
This option feels pretty clean however it would be a breaking change for typescript which might make it a far more difficult proposal
Landed here looking for this feature as well. For what it's worth, this is also implemented in iOS Objective-C using the NS_REQUIRES_SUPER
compiler macro, which annotates a function such that a subclass overriding the function that fails to call super.thatFunction()
will error at compile time.
My main reason for wanting this function is to enable a seamless insertion functionality without the awkward renaming of common well-known functions. When dealing, for example, with the componentDidMount
function in React I think it's most graceful to be able to write:
class Screen extends React.Component {
require_super componentDidMount() {
// ... Do something like logging that the Screen appeared
}
}
class LoginScreen extends Screen {
componentDidMount() {
super.componentDidMount(); // Must call or would cause a TSC error
// ... Do something else like animate in display
}
}
I'd much rather that solution than to have to use final to try and lock down the componentDidMount()
function name at the Screen
class and then require new developers coming on to the project to remember that only when dealing with Screens they actually have to call some project-specific analogous function like thisScreenDidMount()
that is open for overriding. (Though I acknowledge that doing so would achieve the same goal and is, indeed, what Swift, Objective-C's successor language, has opted for as the solution for this problem.)
@zakhenry I second this proposal. I think it could catch bugs before they're written in a number of cases, not just with component lifecycle events
I think the concern about being a breaking change could be addressed by an option in tsconfig.json
, perhaps? Something analogous to strictNullChecks
, like strictOverrideChecks
?
I suggest typescript add annotation like Java.
class Parent {
@supercall // overriding methods with this annotation must call their super methods first, or tsc will throw an error.
protected onCreate() {
// ...
}
}
class Child {
protected onCreate() { // tsc will throw supercall violation error
// does not call the overriden method
}
}
With this approach, typescript doesn't need to consider itself being messed up in grammar. And this annotation feature will open other additional features in way easier and unified way in typescript.
I worry that overloading the @ annotation to represent both compile-time annotations as well as Decorators would create confusion and bugs.
This would be helpful
The same problem I mentioned is described (and solved) here https://blog.gouline.net/enforcing-super-calls-in-android-e9c7b3572b2c
Funny thing, Martin Fowler call this a minor smell. https://martinfowler.com/bliki/CallSuper.html His argument is that the class itself should be built in a way we would never have to think about calling the superclass.
He suggests doing like this
class BaseClass {
onDestroy(){
this.beforeOnDestroy();
// do here the business logic
}
protected beforeOnDestroy(){
// do nothing, this method could have been abstract if we wanted to force subclass to implement the beforeOnDestroy logic
}
}
class subClass extends BaseClass {
protected beforeOnDestroy(){
}
}
I used this idiom (several time especially in C++) which is fine when the base class is yours.
But now we have another problem, nothing prevents us to override onDestroy();
and therefore to break our custom destroy logic. Meaning we would either need a final
keyword or a callsuper
one.
Now the version with callsuper
class BaseClass {
callsuper onDestroy(){
// do here the business logic
}
}
class subClass extends BaseClass {
protected onDestroy(){
// business logic before destroying
this.onDestroy();
}
}
Note that unlike the constructor, the method overriding the callsuper
one should be able to make this super call anywhere. This is mostly because unlike a constructor, we destroy the sub class before the super one.
Sorry, I tapped "Close with comment" by mistake, reopening it.
I've been using this kludge to enforce this in my angular projects:
class BaseClass {
// Returns never to require the child class super.onDestroy()
onDestroy(): never {
return null as never;
}
}
class Child extends BaseClass {
onDestroy() {
// Will fail to compile without this line
return super.onDestroy();
}
}
Really not bad, I would however suggest returning undefined
instead of null
, and perhaps we can just make a type callSuper
to explain better what we are doing.
type callSuper = never;
class BaseClass {
// Returns never to require the child class super.onDestroy()
onDestroy(): callSuper {
return undefined as callSuper;
}
}
class Child extends BaseClass {
onDestroy() {
// Will fail to compile without this line
return super.onDestroy();
}
}
I maintain a library we share internally at our company, and I've had countless issues where somebody implementing just forgot to call super.ngOnInit()
in a subclass, because there's no indication that you would need to without digging into the parent class. That idea for a callSuper
type is going to save everybody a lot of headaches. I'm thinking about implementing the type as a string for enhanced readability:
/**
* If you're here, you probably need to return the parent
* class's implementation, e.g. return super.ngOnInit();
*/
export type CallSuper = "RequireCallSuperMethod";
I'm hoping nobody would type return "RequireCallSuperMethod";
when extending a base class without realizing they need to call the super method 😅
+1 "That idea for a callSuper type is going to save everybody a lot of headaches"
This is an age-old problem and I was hoping TypeScript's maverick nature would provide a mechanism. I could really use it for ngOnInit and ngOnDestroy in subclasses of a generic component component which subscribes / unsubscribes in the superclass. Martin Fowler called "Call Super" enforcement a "minor smell" way back in 2005 but provided a weak and verbose workaround. One solution I used in a huge Java system (usually 5+ inheritance levels deep) was to set flags in the highest-level class to ensure that all the initialization, etc. made its way to the top, then throw an exception if at any point in the lifecycle a flag had not been set. Missed super calls (which happen all the time because they are so easy to forget) are caught during integration testing but it would be nice if the compiler did it instead.
I wonder if https://github.com/Microsoft/TypeScript/issues/2900 could provide an alternative solution that doesn't require a new keyword?
Hey am also look for this in ts. This is my small refactorized scenario !
class Entity<T extends Entity<T>> {
init(): Proxi<this> {
const proxi = store(this);
Object.defineProperty(this, 'Render', { value: <this.renderer entity={proxi} /> }); // cache one renderer with proxi
// others stuff with child shemas ..
return proxi; // return a proxified instance
}
}
export class Plugin extends Entity<Plugin> {
public header: Header; // ex add new shemas to the constructor !
override init() { // the clien want add extra behavior to the entity or component!
this.header = new Header(this.shemas.header).init(); // client overid init with is plugin logic
return super.init(); // user cant return this, because it not proxified, we need to tell him, hey friend! you need return a Proxi<this> with super.init()
}
}
I am desperately search a wait to warn the user , he need return super.init()
because it proxify the instance for the Dom and React renderer
If someone have a idea ? or sugestion !
even if the suggest is not very clean, it can be a temporary solution, otherwise I like the idea of adding tag like required or mustcall
thanks for all your awesome work on ts :❤
Really something as super_required method's modifier is very useful and easy to implement. I added runtime checkings for a few critical methods in my game engine, which checking if all chain of overrided methods was invoked. It is saves a lot of time during debugging, and prevents difficult to find errors. But better solution here is static analyze in compile time. I was sure TS has it already.
I haven't seen this discussed yet, but what mechanism makes the TS compiler bitch about a missing super()
call (TS2377)? A constructor is just a special method, and it looks the driving need (explicitly here, implicitly in a number of other issues) is simply the ability to tell the compiler "please also bitch about it for this method".
I'll grant that the necessity of calling super.method
is a smell, and we're right to oppose anything that increases proliferation of smells when a non smelly alternative exists or can be created. The problem is all these frameworks' lifecycle hooks create a situation where that smell is the most intuitive option for preventing a subclass from preventing the superclass's lifecycle hook from being called.
Hello, I just came to this "call super" problem again today, and I wanted to improve the solution:
class BaseClass {
private static callSuper = Symbol('I am a unique call super');
onDestroy() {
return BaseClass.callSuper;
}
}
class Child extends BaseClass {
onDestroy() {
// Will fail to compile without this line
return super.onDestroy();
}
}
In this way, there is absolutely no way (that I do know or think about) to return anything compliant with the BaseClass signature.
Why ? Because a symbol is unique, it is privately stored into the BaseClass and the only way to get it from the child class is to call the public (or protected) onDestroy
method, using super.onDestroy()
.
If there are 2 method you would like to force calling super, you might have 2 symbol (one for each method) to prevent a method to superCall the other one and bypass the validation… but you get the point (let's keep in mind, the code should be readable…)
bonus… of course you do not have to end with the returning line
class Child extends BaseClass {
onDestroy() {
const superWasCalled = super.onDestroy();
// your stuff here
return superWasCalled;
}
I don't know if it's good to force a subclass to call the superclass method on override
. I usually do the following:
class BaseClass {
private destroy() {
// ... Free resources, etc
this.onDestroy?.()
}
protected onDestroy?(): void
}
class Child extends BaseClass {
onDestroy() {
// ...
}
}
You're right in general, and your solution is closer to the classic Template Method pattern: https://en.m.wikipedia.org/wiki/Template_method_pattern.
The driving use case I believe is frameworks like Angular that already have their base classes and life cycles designed out. You can't do this with the life cycle methods, because there's nothing stopping you from inheriting from a Component and later adding your own ngOnInit unaware.
C++ has the final keyword, which lets you prevent the overriding. That would be sufficient, but you'd still have to architect your own life cycle methods that are then called by the (finalled) Angular ones in the base class.
The ability to slap people with a TS2377 for arbitrary methods would result in much simpler code and stronger guarantees.
On Oct 5, 2021, at 9:27 AM, Filipe Beck @.***> wrote:
I don't know if it's good to force a subclass to call the superclass method on override. I usually do the following:
class BaseClass { private destroy() { // ... Free resources, etc this.onDestroy?.() }
protected onDestroy?(): void }
class Child extends BaseClass { onDestroy() { // ... } } — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android.
@FilipeBeck this is typically a case where forgetting to call the "super" method could create a leak (or any other side effect). As it is hard to debug, we want to avoid this at any cost… @jneuhaus20, did you try my last version ? still a workaround but does the job quite well
@Xample, I like the idea of using a symbol. However, unless you use unique symbol
, then returning any symbol satisfies the parent signature. If you do use unique symbol
you can still cast it, unless it's private. And if it's private, you can't declare the typeof BaseClass.callSuper
signature of onDestroy
. I can't find a way to make symbol work better than any of the other options. If I'm missing something, let me know. Here's the sandbox I was experimenting with, based off of yours.
TypeScript 4.3 comes with noImpicitOverride option, which is good enough for me:
It does not forces you to call super()
, but you have to implicitly use override
keyword, it prevents from unintentional overrides.
Additionally, I use this pattern, that prevents from overriding onDestroy()
class BaseComponent
{
protected readonly destroy$: IObservable = new Subject();
readonly onDestroy = () => {
(this.destroy$ as Subject).onNext();
(this.destroy$ as Subject).onComplete();
}
}
auto unsubscribing observable using .takeUntil(this.destroy$)
pattern
class Child extends BaseComponent {
onInit() {
myService.someObservable()
.takeUntil(this.destroy$) //auto unsubscribe
.subscribe(() => {
})
}
}
alternativelly, you still can subscribe to this.destroy$ to peform custom cleanup:
this.destroy$.subscribe(() => {
//your cleanup here
})
@bmcbarron A symbol is always unique but I did not try returning another symbol. And yes this is possible to cheat the type using typeof, among other hacks. I agree, the solution is to be considered a workaround.
@Liero It's my understanding that this thread is about being warned that you've not called the superclass method correctly, so the current TS implementation doesn't satisfy that.
@Xample The problem with this is that you have to return a specific thing. What if your methods return void or should return something useful?
My use case seems slightly different to the ones mentioned so far here. My superclass has a "setup" method which can be overridden by subclasses and should be called on or near instantiation. I have a scenario like so:
class Parent {
setup () {
/* do some parent setup */
}
}
class Child extends Parent {
setup () {
super.setup()
/* do some child setup /*
}
}
class Baby extends Child {
setup () {
super.setup()
/* do some baby setup /*
}
}
Note that in my methods the superclass method is called first. If I miss one of these I should see an error, but obviously don't. And in your workaround I'm restricted to always calling the superclass method last (unless I'm missing something?).
Unfortunately right now I'm having to listen for a lack of a call via the parent class, which is ugly and prone to errors if the calls take longer than expected. And obviously you cannot see issues until runtime:
class Parent {
protected superSetupCalled = false
constructor () {
setTimeout(() => {
if (!this.superSetupCalled)
throw new Error('Parent super.setup() not called; have you implemented your subclass properly?')
}, 3000 /* some arbitrary time */)
}
setup () {
this.superSetupCalled = true
/* do some parent setup */
}
}
I really hope TS considers a @required keyword or something similar; such that I can ensure all superclass methods are called correctly by their subclasses.
I'm really liking the override
implementation. Just need final
now. Very similar to what's available in Swift.
I know the original issue hasn't been resolved, but with final
you can do something. Maybe not as elegant with more than one level of inheritance. But to be fair, I don't know of a solution in other languages that has this - other than using the constructor (I get sometimes you can't).
final should allowed override when using super.method()
temp solution
/** @final */
public init(){
// private stuff ....
this.onInit(); // implementation stuff ...
}
So when mouse over with your IDE, jsdoc know we should not override, or override allowed by adding super()
but it a ~8yr feature request , and so many treads ask for it .
no more hope :(
we need a heroes plz
sry for poke, but any chance to add this to Milestone? or have official answer why this is not possible in ts ? @ahejlsberg @amcasey @jakebailey
@briancodes Maybe I'm doing something wrong, but the solution you provided seems not to be working in the Angular v15. (note that I replaced ngOnDestroy
with the ngOnInit
so it's called initially, but that doesn't change anything) https://stackblitz.com/edit/angular-ivy-umqhty?file=src/app/hello.base.component.ts
@lukasmatta your subclass (hello.component.ts
) doesn't have a constructor
and so isn't calling super()
If using ts-loader, there is this: ts-loader-forcesupertransformer
Today I faced the following code
The problem is that, unlike a constructor, there is no way to force a method overriding another one to call the parent "super" function.
I wished we had a "concrete"* keyword to let the user know he must call the super method.
In another language, I could have used the
final
keyword to prevent overriding the method but then… no overriding allowed neither.