Skip to content

Suggestion: Prevent hiding super class properties and methods #9060

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

Closed
MKuenzi opened this issue Jun 9, 2016 · 9 comments
Closed

Suggestion: Prevent hiding super class properties and methods #9060

MKuenzi opened this issue Jun 9, 2016 · 9 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@MKuenzi
Copy link

MKuenzi commented Jun 9, 2016

Code

class A
{
   public member = {};
   public click = () =>
   {
   }

   constructor()
   {
      member.a = "Member";
      document.body.addEventListener("click", this.click);
   }
}

class B extends A
{
   public member = {};
   public click = () =>
   {
       // Implementer of subclass expects this to get called on body click
   }

   constructor()
   {
      super();
      console.log(this.member.a) // would print undefined
   }
}

It would be nice if we were able to generate a warning or error to say that B's member is hiding A's member, as well as the fact that B's click is hiding A's click. I'm not sure whether a new keyword would be appropriate, or just an option in tsconfig.json to generate an error or warning would make me happy.

Not having this has lead me to some head scratching to why things aren't working as I rearrange how things inherit from each other due to me not noticing that I have redefined a method or property in the subclass.

@RyanCavanaugh
Copy link
Member

#2000 tracks the override keyword for handling this problem

@MKuenzi
Copy link
Author

MKuenzi commented Jun 9, 2016

I've read through that issue and I feel that my problem is a little different. For me I'd rather be able to designate on my base class that something shouldn't be overridden, maybe #1534 is closer to what I'm thinking, although I'd also like the ability to seal an instance property.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 9, 2016

I do not think this is #2000, this is rather different. this is akin to using a virtual function in the constructor of the base class. usually not a good recipe, see some comments for C++ http://stackoverflow.com/questions/962132/calling-virtual-functions-inside-constructors and for C# http://stackoverflow.com/questions/119506/virtual-member-call-in-a-constructor.

I would suggest either using capture in the call site:

   constructor()
   {
      member.a = "Member";
      document.body.addEventListener("click", ()=> { this.click() } );
   }
}

or making this a constructor argument.

@mhegazy mhegazy added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Jun 9, 2016
@MKuenzi
Copy link
Author

MKuenzi commented Jun 9, 2016

I was thinking of the issue I'm having similar to hiding inherited members in C#

When used as a declaration modifier, the new keyword explicitly hides a member that is inherited from a base class. When you hide an inherited member, the derived version of the member replaces the base class version. Although you can hide members without using the new modifier, you get a compiler warning. If you use new to explicitly hide a member, it suppresses this warning.

From here:
https://msdn.microsoft.com/en-us/library/435f1dw2.aspx

I understand what you mean, however my goal is to prevent the user from redefining the function/property in the subclass.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 9, 2016

still does not solve your issue. the problem is not that you are shadowing a member. the problem is you are using it in the base constructor before the new member have been installed on the object.

@MKuenzi
Copy link
Author

MKuenzi commented Jun 10, 2016

We use a lot of observable objects in our ViewModels so we like to associate a callback for when they are changed in our constructor. It feels right to do this in the constructor, Otherwise I'd have to call some setupEvents() method after the object is constructed.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 10, 2016

then you should capture the this and invoke the handler when needed, instead of capturing the handler directelly, i.e.:

document.body.addEventListener("click", ()=> { this.click() } );

instead of:

document.body.addEventListener("click",  this.click );

@MKuenzi
Copy link
Author

MKuenzi commented Jun 13, 2016

The only issue I have with this is I wouldn't be able to remove the event listener.

@kitsonk
Copy link
Contributor

kitsonk commented Jun 13, 2016

const listener = ()=> { this.click() };
document.body.addEventListener("click",  listener);
document.body.removeEventListener("click", listener);

@mhegazy mhegazy closed this as completed Jun 13, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

4 participants