Static getter executed on class inheritance #8420

Activity

1. malibuzios commented on May 2, 2016

   

   malibuzios

   on May 2, 2016

   Last edited by malibuzios

   More actions

   Tried it on 1.9.0-dev.20160501 on ES5 target, same result, the _extends helper looks like this:

   var __extends = (this && this.__extends) || function (d, b) {
       for (var p in b) 
           if (b.hasOwnProperty(p))
               d[p] = b[p];
       function __() { this.constructor = d; }
       d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
   };

   I guess perhaps the assignment here could check if the member is a property and then use something like Object.getOwnPropertyDescriptor and then Object.defineProperty (or alternatively just Object.assign the whole thing if possible (but that wouldn't work with non-enumerable members, see below)).

   Something like:

   var __extends = (this && this.__extends) || function (d, b) {
       for (var p in b) 
           if (b.hasOwnProperty(p)) {
               var descriptor = Object.getOwnPropertyDescriptor(b, p);
   
               if (typeof descriptor.get === "function" ||
                   typeof descriptor.set === "function") {
                   Object.defineProperty(d, p, descriptor)
               }
               else {
                   d[p] = b[p]; // This is only for performance, 'defineProperty' can be used for all cases
               }
           }
       function __() { this.constructor = d; }
       d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
   };

   In any case, with #3610 the current solution wouldn't work anyway, and for methods as well, as it only accounts for enumerable properties. They would need to use something like Object.getOwnPropertyNames(b) instead, so I guess this would be changed to something like:

   var __extends = (this && this.__extends) || function (d, b) {
       var props = Object.getOwnPropertyNames(b);
   
       for (var i = 0; i < props.length; i++) { 
           var descriptor = Object.getOwnPropertyDescriptor(b, props[i]);
           Object.defineProperty(d, props[i], descriptor);
       }
       function __() { this.constructor = d; }
       d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
   };

   I'm surprised this hasn't been noticed yet?

   Edit: I didn't actually notice this was about static methods. I was mostly referring to instance ones, so I'm not 100% sure if #3610 includes them as well - it is possible but I haven't checked..
2. mhegazy commented on May 2, 2016

   

   Mohamed Hegazy (mhegazy)

   on May 2, 2016

   Contributor

   More actions

   This is a duplicate of #1520. please see reply in #1520 (comment).
3. Mohamed Hegazy (mhegazy)

   closed this as completedon May 2, 2016
4. Mohamed Hegazy (mhegazy)

   added

   DuplicateAn existing issue was already createdAn existing issue was already created

   on May 2, 2016
5. malibuzios commented on May 2, 2016

   

   malibuzios

   on May 2, 2016

   Last edited by malibuzios

   More actions

   Mohamed Hegazy (@mhegazy)

   Seems like it would be fixed anyway when #3610 is introduced? (I mentioned that in my comment?)
6. malibuzios commented on May 2, 2016

   

   malibuzios

   on May 2, 2016

   More actions

   Well, static getters/setters/methods seem to actually be enumerable based on my tests, so I'm not sure if it would be fixed any time soon... :'(
7. malibuzios commented on May 2, 2016

   

   malibuzios

   on May 2, 2016

   Last edited by malibuzios

   More actions

   Mohamed Hegazy (@mhegazy)

   I think you should seriously consider disallowing inheritance of classes with static getters and setters for ES3/ES5 targets. There's not really much of a point of having it if it doesn't actually work?
8. mhegazy commented on May 2, 2016

   

   Mohamed Hegazy (mhegazy)

   on May 2, 2016

   Contributor

   More actions

   That would be a breaking change. Today you can do this by overriding the __extends helper, which is a supported scenario.
9. malibuzios commented on May 2, 2016

   

   malibuzios

   on May 2, 2016

   Last edited by malibuzios

   More actions

   Mohamed Hegazy (@mhegazy)

   How is it a breaking change to move from something that doesn't work to something that simply tells you it doesn't work? seems like an enhancement maybe?

   Anyway, many people come from languages like C# where static classes are the main way to describe global functionality. I tend to use them this way myself. I do use static getters and setters and care a lot about ES5 compatibility.

   (I noticed that in the compiler you almost don't use classes at all. Maybe if you've used it more extensively personally it would feel more important..?)

   It could be true that the pattern is not statistically common but the more popular the language becomes, the more things that were once uncommon or seemed insignificant would start to come to light. I think this is a positive thing that is a sign of success, not failure. Also the more people start to truly understand and care about the language, they would want you to address some relatively minor things that might not seemed that important initially (why doesn't it error if..? etc), a lot of the time simply because they like to feel that they are using a 'polished' and 'well-rounded' product.

   Also, people's use of the language will become more sophisticated with time as well, and things like generics, index signatures, static setter/getters, generic constraints etc. will become more widely used, so the impact of some of the 'holes' over there may also become more significant..
10. malibuzios commented on May 3, 2016

    

    malibuzios

    on May 3, 2016

    Last edited by malibuzios

    More actions

    I don't think overriding helpers is something most people would want to deal with. It feels like something someone would do as an experiment maybe. In any case they would have to do this separately for every one of their projects and adapt it to the particular build system used in it, and somehow make sure it is evaluated before anything else (for example, I use a glob based build system so I don't currently have a precise order guarantee as the file list is auto-generated).

    I could give you a pull request for:

    Line 341 in emitter.ts

    But I'm almost 100% sure it would be rejected, so I'm not even going to try..
11. microsoft

    locked and limited conversation to collaborators on Jun 19, 2018