-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
keyword to force calling the super on any method #21388
Comments
Instead of requiring overriders to remember to call 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 |
The framework is giving you the interface. You would add |
A workaround: Declare that 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 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 |
@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 One way to do this, which works if the sub classes call In the Base Class
|
The keyword could be |
@lukescott like required in swift ? 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 |
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 |
For example, Java and C# does not suffer from this problem, because they require to explicitly use 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 |
Is this summary correct?
|
@Xample I feel like in the 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 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 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 |
@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 |
I suggest typescript add annotation like Java.
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. |
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 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. Now the version with 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 |
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 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 /**
* 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 |
+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 #2900 could provide an alternative solution that doesn't require a new keyword? |
Hey am also look for this in ts. 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 |
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 I'll grant that the necessity of calling |
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. 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 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… |
@Xample, I like the idea of using a symbol. However, unless you use |
TypeScript 4.3 comes with noImpicitOverride option, which is good enough for me: It does not forces you to call 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();
}
}
class Child extends BaseComponent {
onInit() {
myService.someObservable()
.takeUntil(this.destroy$) //auto unsubscribe
.subscribe(() => {
})
}
}
this.destroy$.subscribe(() => {
//your cleanup here
}) |
@bmcbarron |
@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 I know the original issue hasn't been resolved, but with |
final should allowed override when using |
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 sry for poke, but any chance to add this to Milestone? or have official answer why this is not possible in ts ? |
@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 |
@lukasmatta your subclass ( |
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.The text was updated successfully, but these errors were encountered: