Typescript: Allow to call getter/setter logic When subclassing

[mwisnicki]

http://typescript.codeplex.com/workitem/645

At the moment it's not possible to call super getter/setter when overriding properties:

class A {
    private _property: string;
    get property() { return this._property; }
    set property(value: string) { this._property = value }
}

class B extends A {
    set property(value: string) {
        super.property = value + " addition";
    }
}

[RyanCavanaugh]

The problem is that we don't surface the fact that a property is a getter/setter in the type information, and it's impossible to emit code here that would work even if the base class's definition was _not_ a getter/setter. Using 'virtual' properties in JavaScript is generally not a good idea for this reason.

/* MyDeclFile.d.ts */
declare class A {
  x: number; // actually implemented as getter/setter
  y: number; // actually implemented as field
}

/* MyConsumer.ts */
class B {
   get x() { return super.x; } // Could theoretically emit code for this
   get y() { return super.y; } // Cannot work; there is no 'super.y' to refer to
}

[fdecampredon]

It's part of es6 class, so it seems pretty important to me, also generating something that throws an error in case of missing super setter and return undefined in case of missing super getter (like what traceur does https://github.com/google/traceur-compiler/blob/master/src/runtime/classes.js#L43-L60) seems pretty much standard and compliant with what future es6 engine will do.

[RyanCavanaugh]

The point about ES6 is very important. We'll need to figure out what that means.

[ahejlsberg]

I think it means that we'll support super.property = value and value = super.property to access inherited accessors _when emitting code for ES6_. However, I don't know of a good way to emulate this behavior with ES5 style property accessors that are defined using Object.defineProperty. We had pretty extensive discussions about this early on in the TypeScript design and eventually decided that there was no good solution. The recommended workaround is to define property getters and setters that delegate the actual work to methods (which can then be overridden as normal).

[fdecampredon]

Just for info, why the traceur runtime code portion that I pointed is not considered _a good solution_ ? Like the __extends function could we not have __superGet and __superSet ?

[ahejlsberg]

I have two concerns: First, the performance of a __superGet or __superSet helper is likely to be orders of magnitude slower than a regular property access, yet there is nothing in the code to alert you to that fact. Second, one of the attractive aspects of TypeScript is that it doesn't need a library of helpers--it's just JavaScript. The _only_ helper we have is __extends, the only time we inject it is when a file contains a derived class, and the only reason we think it is acceptable is because this is a very high value scenario. This would add two new injection points. Perhaps that's ok, but as a strategy it doesn't really scale and I'm not sure the feature adds enough value.

[mwisnicki]

What if we lookup getters and setters only once, right before defining property ? For example like this.

[duncanmak]

Another possibility is to only allow it when emitting in ES6?

[RandScullard]

When I try a super property reference in the Playground, it gives me a compiler error but seems to work fine at runtime. Is this just a question of eliminating the compiler error? I'm sure I'm missing something, so please enlighten me!

Playground

[RandScullard]

Sorry, I just answered my own question. It only works in that trivial example, where there is no reference to the instance state of the derived class.

[ghost]

Couldn't you just change the emission of the generated code during conversion to java style accessor methods, so you get to keep the nice syntatic sugar in TS with expected and still understandable output in the final JS? Seems like an easy and not necessarily "bad" solution. My $0.02.

[RyanCavanaugh]

Emitting get_Foo/set_Foo methods would be very problematic. You have to not only rewrite the property itself, but all references to it, and then you have one interface when you're inside the compilation unit (foo.x) and a different one when you're outside the compilation unit (foo.get_x()). No one would expect that changing compilation unit boundaries would mean you'd have to completely rewrite your code.

It wouldn't be feasible for the compiler to do this for you, because there's no information when looking at a declare class about if the property was implemented as a getter/setter (or if the class is actually a TypeScript class at all!). At the point where we start adding information about the implementation of a particular property, the hard part of the problem goes away _anyway_, so making explicit get/set methods doesn't actually solve anything.

[mwisnicki]

There is no need to rewrite references to properties and have 2 interfaces if you also generate ES5 property that forwards to getter/setter.

[ghost]

Alright well that settles that. I guess that's why I don't work for microsoft working on a language spec and compiler. ;)

[ghost]

Or rather I should say, I shouldn't have chipped in after just barely starting to toy with TS. :)

[mtraynham]

Has anyone considered just copying the super implementation to the child class and giving it a different variable name? If your class overrides the property, provide one renamed copied "private" implementation to get the super property and one to get the overridden subclass property?

class A {
    public id: string;
    private _name: string;

    public get name(): string {
        return this.name;
    }
    public set name(name: string) {
        this._name = name;
    }
}

class B extends A {
    public get id(): string {
        return super.id + 'foo';
    }
    public set id(id: string) {
        super.id = id;
    }

    public get name(): string {
        return super.name + 'bar';
    }
    public set name(name: string) {
        super.name = name;
    }
}

Class B Becomes:

function B(_super) {
    Object.defineProperty(B.prototype, "id", {
        get: function () {
            return this._id + 'bar';
        },
        set: function (id) {
            this._id = id;
        },
        enumerable: true,
        configurable: true
    });

   Object.defineProperty(B.prototype, "_name", {
        get: function () {
            // super implementation
            return __name;
        },
        set: function (name) {
            // super implementation
            this.__name = name;
        },
        enumerable: false,
        configurable: false
    });

    Object.defineProperty(B.prototype, "name", {
        get: function () {
            return this._name + 'bar';
        },
        set: function (name) {
            this._name = name;
        },
        enumerable: true,
        configurable: true
    });
}

To address @RyanCavanaugh 's point, you could just move y on the subclass to _y, as long as the variable name doesn't conflict with something previously defined by the user.

Maybe I'm missing something though...

[RyanCavanaugh]

It's not at all guaranteed that you have access to the base class's implementation code.

[mtraynham]

Well there is Object.getOwnPropertyDescriptor(A.prototype, 'name') which returns the implementation details for A's name implementation, albeit it is more of a run-time feature rather than compile time.

[mwisnicki]

So what's wrong with get_Foo/set_Foo + Object.defineProperty, where super.foo gets rewritten to setter/getter while any other reference is left as is ?

[jiaweihli]

The workarounds seem to boil down to:

• [application maintenance issue] duplicating getter/setter implementations from parent classes
• [performance issue, non-idiomatic] using java-style setXYZ / getXYZ methods instead of native ES5 methods
• [application maintenance issue] using a reflective superGet method, e.g.

var getPropertyDescriptor = function(subject, name) {
  var pd = Object.getOwnPropertyDescriptor(subject, name);
  var proto = Object.getPrototypeOf(subject);
  while (pd === undefined && proto !== null) {
    pd = Object.getOwnPropertyDescriptor(proto, name);
    proto = Object.getPrototypeOf(proto);
  }
  return pd;
};

class Test {
  superGet(property) {
    return getPropertyDescriptor(Object.getPrototypeOf(this.constructor.prototype), property).get();
  }

  get foo(): number { return 5; }
}

class Test2 extends Test { }

class Test3 extends Test2 { }

class Test4 extends Test3 {
  get foo(): number {
    return this.superGet('foo') + 1;
  }
}

var t = new Test4;
console.log(t.foo);

(^ This could be sped up and avoid some Object.getPrototypeOf calls if we had access to the _super.prototype variable via the super keyword.)

Combining the third option with @fdecampredon's suggestion:

also generating something that throws an error in case of missing super setter and return undefined in case of missing super getter (like what traceur does https://github.com/google/traceur-compiler/blob/master/src/runtime/classes.js#L43-L60) seems pretty much standard and compliant with what future es6 engine will do.

and @mwisnicki's suggestion:

What if we lookup getters and setters only once, right before defining property ? For example like this.

and tweaking @duncanmak's suggestion:

Another possibility is to only allow it when emitting in ES6ES5?

seems viable in the compiler (eliminating stringy / maintenance issues), and would cover the majority of use cases.

EDIT: Bumped code.
EDIT 2: This is an implementation I'm now using: https://gist.github.com/jiaweihli/807b56a75909a628fe7e

[vladima]

5860 lifts the restriction for ES6, we still need a separate proposal for the downlevel emit

[jiaweihli]

@vladima I have a working solution (built on top of my gist, fixes a few bugs) for ES5, which we're running in production. It uses 2 external dependencies, but these aren't strictly necessary and their functionality can be inlined. Should I open a new issue?

[jiaweihli]

^ Bump @vladima

[pwnall]

@vladima Can you comment as to whether a proposal for the downlevel emit based on @jiaweihli's gist would have a chance of being accepted?

I don't think there is any solution that mitigates @ahejlsberg's first concern. However, in response to the second concern, TypeScript now has at least two more helpers to support async/await. Also, this path will not descend into madness, as there is a finite set of features in ES6 that can be polyfilled in ES5.

[amcdnl]

Is there any open issues on this being merged downward to ES5? Moved from Babel to TS and had a lot of this going on, ended up doing something like:

  set sourcePosition(val: number) {
    // was working for babel
    // super.sourcePosition = val;

    // hack for ts :(
    Object.getOwnPropertyDescriptor(
      Object.getPrototypeOf(DirectedEdgeView.prototype), 'sourcePosition')
        .set.call(this, val);

    this.updateArrowPosition();
  }

[jpwiddy]

Any proposal to bring this to ES5? It's very obscure, it doesn't work as expected. I'd really like this to be resolved.

[jiaweihli]

@vladima @RyanCavanaugh @ahejlsberg

Any comments on my proposal for the downlevel emit?

[jpwiddy]

@jiaweihli your proposal still doesn't work completely. It saves the value to the prototype, not the instance. So the super class's prototype will incorrectly have the value.

var getPropertyDescriptor = function(subject, name) {
  var pd = Object.getOwnPropertyDescriptor(subject, name);
  var proto = Object.getPrototypeOf(subject);
  while (pd === undefined && proto !== null) {
    pd = Object.getOwnPropertyDescriptor(proto, name);
    proto = Object.getPrototypeOf(proto);
  }
  return pd;
};

class Test {
    private _foo: any;
  superGet(property) {
    return getPropertyDescriptor(Object.getPrototypeOf(this.constructor.prototype), property).get();
    }
  superSet(property, value) {
    return getPropertyDescriptor(Object.getPrototypeOf(this.constructor.prototype), property).set(value);
    }

  constructor() { }

  set foo(val) {
      this._foo = val;
  }
  get foo(): number { return this._foo; }
}

class Test2 extends Test { }

class Test3 extends Test2 { }

class Test4 extends Test3 {
  get foo(): any {
      return this.superGet('foo');
  }
  set foo(val) {
      this.superSet('foo', val);
  }
}

var t = new Test4();
var t2 = new Test4();

t.foo = 123;
t2.foo = 321;

console.log(t.foo);  // undefined
console.log(t2.foo);  // undefined

console.log(Test4);  // has no foo value
console.log(Test);  // has no foo value 

[jiaweihli]

@jpwiddy
Sorry for the slow update.

The proposal you tested is obsolete and incorrect, like you discovered 😛 . Here's the new (and always up-to-date) proposal: https://gist.github.com/jiaweihli/807b56a75909a628fe7e

The new version correctly works with the example you gave. This uses 2 external libraries at the moment for convenience, but these aren't strictly necessary and their functionality can be inlined.

[jpwiddy]

Hah no worries! Thanks for the updated gist! Looks better, I'd like to see the external dependencies alleviated in case this is pulled into TS. It'd make it much easier to merge in.

Can we get this issue opened back up?

[Hotell]

This is a big deal breaker with Web Components and SkateJS. https://skatejs.gitbooks.io/skatejs/content/docs/api/Component.html#static-props

can we re-open this pls @mhegazy @RyanCavanaugh ?

Typescript version: 2.2

Related issue:
https://github.com/skatejs/skatejs/issues/1077

Playground: https://goo.gl/f7ALUx

If user want's to leverage mixins ( added in TS 2.2 ) this error is preventing to compile without errors which is very unfortunate:

Errored part:

   static get props(): ComponentProps<any, DisabledProps> {
      return {
        ...super.props,
        disabled: prop.boolean( { attribute: true } )
      };

interface Constructable<T> {
  readonly props: ComponentProps<any, any>;
  readonly observedAttributes: string[];
  readonly is: string;
  new ( ...args: any[] ): T,
}

export function Disabled<BC extends Constructable<{}>>( Base: BC ) {
  return class extends Base {
    static get props(): ComponentProps<any, DisabledProps> {
      return {
        ...super.props,
        disabled: prop.boolean( { attribute: true } )
      };
    }
    disabled?: boolean;
  };
}

class MyComponent extends Disabled( Component )<{}>{
  static get props(){
    return {
      color: {}
    }
  }
  renderCallback(){
     const {disabled,color} = this;
     return (
       <button disabled={disabled} style={{color}}>Hello</button>
     )
   }
}

[Neewd]

Hi,

Any update about this issue ?

[kriszyp]

For the record, when we transitioned from Babel to TypeScript, I wrote this custom transformer + runtime patch. It's a bit of hack from a transformation perspective, but I believe it is semantically correct in behavior (and preserved our super accessor functionality as compiled with babel):
(before) transformer:

const ts = require('typescript')

function visitor(ctx, sf) {

  function visit(node) {
    if (ts.isBinaryExpression(node) && node.operatorToken.kind === ts.SyntaxKind.EqualsToken && ts.isPropertyAccessExpression(node.left) && node.left.expression.kind === ts.SyntaxKind.SuperKeyword) {
      // super.property = value
      // transform to:
      // Object.superSet(_super.prototype, propertyName, this, value)
      return ts.createCall(
        ts.createPropertyAccess(
          ts.createIdentifier('Object'),
          'superSet'),
        [],
        [ts.createSuper(), ts.createLiteral(node.left.name), ts.createThis(), node.right])
    }
    if (ts.isPropertyAccessExpression(node) && node.expression.kind === ts.SyntaxKind.SuperKeyword &&
        // make sure we *don't* transform super.method() calls
        !(ts.isCallExpression(node.parent) && node.parent.expression === node)) {
      // super.property
      // transform to:
      // Object.superGet(_super.prototype, propertyName, this)
      return ts.createCall(
        ts.createPropertyAccess(
          ts.createIdentifier('Object'),
          'superGet'),
        [],
        [ts.createSuper(), ts.createLiteral(node.name), ts.createThis()])
    }
    return ts.visitEachChild(node, visit, ctx)
  }
  return visit
}

module.exports = function(/*opts?: Opts*/) {
  return (ctx) => {
    return (sf) => ts.visitNode(sf, visitor(ctx, sf))
  }
}

runtime patch:

Object.superGet = (superPrototype, name, instance) => {
  do {
    let descriptor = Object.getOwnPropertyDescriptor(superPrototype, name)
    if (descriptor) {
      return descriptor.get ? descriptor.get.call(instance) : descriptor.value
    }
  } while ((superPrototype = Object.getPrototypeOf(superPrototype)))
}

Object.superSet = (superPrototype, name, instance, value) => {
  do {
    let descriptor = Object.getOwnPropertyDescriptor(superPrototype, name)
    if (descriptor) {
      return descriptor.set && descriptor.set.call(instance, value)
    }
  } while ((superPrototype = Object.getPrototypeOf(superPrototype)))
  Object.defineProperty(instance, name, {
    value,
    configurable: true,
    enumerable: true,
    writable: true
  })
}

The runtime semantics require no knowledge of types or super class implementations. Obviously to put this in TypeScript proper, you would want that code to be generated, and/or in tslib, not patched on Object, though. As a before transformer, this relies on the fact that generating a super token will get transformed to _super.prototype in the later downlevel transformers. Naturally, would greatly prefer this in TypeScript itself, so maybe this will help.

[brianmhunt]

Thanks for reopening, @RyanCavanaugh — this is a problematic issue and readily worked around, per e.g. the patch by @kriszyp

[RyanCavanaugh]

Long story short - after five years of not having this and people surviving despite it, it seems like ES6 adoption is catching up fast enough that the narrow window of people who target ES5 (rather than 3 for maximum compat or 6 for smallest emit delta) is closing to the point where we don't want to accept a big chunk of helper emit at this point. Open to data otherwise but it just doesn't seem common enough to warrant a big change in the emitter.

[brianmhunt]

Just a data point: This is a blocker to using Typescript to compile Knockout.js version 4. The depth of workaround needed is uncertain, but it feels problematic.

Cross-linking https://github.com/knockout/tko/issues/21

[thomashilzendegen]

I'm just curious, but when I put the following code into the playground, the output JS seems legit to me (and it works):

class Foo {
    get bar() {
        return 'foo';
    }
}

class Bar extends Foo {
    get bar() {
        return super.bar + 'bar';
    }
}

const x = new Bar();
alert(x.bar);

Does this mean, access to super's property works, but the compiler is still complaining?

[brianmhunt]

@ThomasdenH It's references to this that are broken. Try:

class Foo {
    constructor () { this.x = 'dee' }
    get bar() {
        return 'foo' + this.x;
    }
}

class Bar extends Foo {
    get bar() {
        return super.bar + 'bar';
    }
}

const foo = new Foo()
const bar = new Bar();
alert(`foo: ${foo.bar}, bar: ${bar.bar}`)

The output above is foo: foodee, bar: barundefinedbar, but when run in an ES6 engine (i.e. the correct outcome) it is foo: foodee, bar: foodeebar.

[thomashilzendegen]

@brianmhunt Brrr, I see. Good catch.

[SlurpTheo]

The output above is foo: foodee, bar: barundefinedbar, but when run in an ES6 engine (i.e. the correct outcome) it is foo: foodee, bar: bardeebar.

@brianmhunt Little typo there:

The output above is foo: foodee, bar: fooundefinedbar, but when run in an ES6 engine (i.e. the correct outcome) it is foo: foodee, bar: foodeebar.

[brianmhunt]

@SlurpTheo Yes, you're correct; edited my comment to fix; thanks!