Skip to content
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

Allow to call getter/setter logic When subclassing #338

Closed
mwisnicki opened this issue Aug 2, 2014 · 41 comments · Fixed by #5860
Closed

Allow to call getter/setter logic When subclassing #338

mwisnicki opened this issue Aug 2, 2014 · 41 comments · Fixed by #5860
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript

Comments

@mwisnicki
Copy link

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
Copy link
Member

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
Copy link

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
Copy link
Member

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

@sophiajt sophiajt self-assigned this Aug 11, 2014
@sophiajt sophiajt added this to the TypeScript 2.0 milestone Aug 11, 2014
@sophiajt sophiajt modified the milestones: TypeScript 2.0, TypeScript 1.2 Sep 2, 2014
@ahejlsberg
Copy link
Member

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
Copy link

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
Copy link
Member

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
Copy link
Author

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

@duncanmak
Copy link

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

@sophiajt sophiajt modified the milestones: Suggestions, TypeScript 1.3, TypeScript 2.0 Sep 8, 2014
@RandScullard
Copy link

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
Copy link

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.

@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus Suggestion An idea for TypeScript and removed Fixed A PR has been merged for this issue Spec Issues related to the TypeScript language specification labels Mar 8, 2017
@RyanCavanaugh RyanCavanaugh removed this from the TypeScript 1.8 milestone Mar 8, 2017
@RyanCavanaugh RyanCavanaugh reopened this Mar 8, 2017
@Neewd
Copy link

Neewd commented Sep 28, 2017

Hi,

Any update about this issue ?

@kriszyp
Copy link
Contributor

kriszyp commented Oct 5, 2017

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
Copy link

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

@RyanCavanaugh RyanCavanaugh added Declined The issue was declined as something which matches the TypeScript vision and removed In Discussion Not yet reached consensus labels Nov 28, 2017
@RyanCavanaugh
Copy link
Member

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
Copy link

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 knockout/tko#21

@thomashilzendegen
Copy link

thomashilzendegen commented Dec 15, 2017

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
Copy link

brianmhunt commented Dec 15, 2017

@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
Copy link

@brianmhunt Brrr, I see. Good catch.

@SlurpTheo
Copy link

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
Copy link

brianmhunt commented Dec 15, 2017

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.