Expected behaviour of __extends with static accessors?

The following code:

var i = 0, j = 0;


class Base {
    get instanceAccessor() { return ++i; }
    static get staticAccessor() { return ++j; }
    toString = () => 'instance: ' + this.instanceAccessor + '   static: ' + Base.staticAccessor;
}


class Derived extends Base {
    toString = () => 'instance: ' + this.instanceAccessor + '   static: ' + Derived.staticAccessor;
}


console.log('Base:\n%s\n%s\n%s', new Base(), new Base(), new Base());
console.log('Derived:\n%s\n%s\n%s', new Derived(), new Derived(), new Derived());

generates the following output (with the v1.3 compiler):

Base:
instance: 1   static: 2
instance: 2   static: 3
instance: 3   static: 4
Derived:
instance: 4   static: 1
instance: 5   static: 1
instance: 6   static: 1

Clearly Derived.staticAccessor is no longer an accessor, but a 'snapshot' of the value of Base.staticAccessor when the Derived constructor function called __extends.

Just wondering if this is the expected/correct behaviour?

PS. I haven't run into this problem 'in the wild' as I rarely use classes or accessors, but merely came across a rant/blog post about another compile-to-JS language doing something similar, and was curious how TypeScript handled it.

It's not correct, but it is 'expected' (i.e. we've known about the problem but have chosen not to fix it).

Our take was that the actually prevalence of this pattern in the wild is ~0%, and that the amount of extra code (and perf hit) you'd need in __extends wasn't worth it to support. For people who actually do have derived static getters they want to work, they can supply their own __extends function that does all the legwork of calling Object.getOwnPropertyDescriptor and doing the right thing. As far as I know, no one has hit this in real code (or at least no one has told us about it based on actual experience) so it seems to be the right call for now.

Ryan Cavanaugh (@RyanCavanaugh)

Sounds reasonable! Especially given that programs can supply their own version of __extends if needed.

How does one supply a custom __extends function (preferrably suppressing the generation of the default one)? I can't see anything in the spec about doing this.

EDIT: answered my own question: Ryan Cavanaugh (@RyanCavanaugh) 's PR #1356 takes care of this.

You don't really need the PR unless you're trying to get a function through an import (kind of an odd scenario and the PR still needs some work) -- the __extends emit starts with var __extends = __extends ||, so if there's already a function defined, that one will be given preference.

Got it. I guess the PR just makes the generated code closer to what one would write by hand (ie not emitting unused __extends functions all over the place).

Same problem here. Maybe you should just admit in the spec that static accessors aren't supported?
Or else, just do the right thing...
Generate an __extendsProper, as well, when you know a class needs it. Use it to extend classes with known base static getters...

Ryan Cavanaugh (@RyanCavanaugh)

Ryan Cavanaugh (@RyanCavanaugh)

I'm successfully using this modified version I wrote for __extends:

var __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];
            }
        }

    function __() { this.constructor = d; }
    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};

I'm interested to know what are the possible performance implications of this? (since jsPerf is down I cannot really share a benchmark but I may write a custom one at some point)

This is only evaluated once per each inheriting class during the initialization of the program. I'm aware of possible performance implication of properties and methods initialized using Object.defineProperty but this is not the case here, because I optimized to only use it for accessors, which are defined using it in anyway.

And I even have tests for it (for personal use), which don't pass with the current helper:

namespace ExtendTests {
    class Base {
        static counter = 1;

        static get counterAccessor() {
            return this.counter;
        }

        static set counterAccessor(val: number) {
            this.counter = val;
        }

        static incrementCounter() {
            this.counter++;
        }
    }

    class Derived extends Base {
    }

    describe("__extend", () => {
        it("Inherits a static getter and setter correctly", () => {
            expect(Derived.counterAccessor).toEqual(1);
            Derived.incrementCounter();
            expect(Derived.counterAccessor).toEqual(2);

            Derived.counterAccessor = 10;
            expect(Derived.counter).toEqual(10);
        });
    });
}

This also makes me concerned that the compiler code may not include a single test containing a inherited class having a static getter or setter (or it does include but it repeatedly fails on ES5 mode). I mean, there could be other issues in this scenario that are not covered, and this pattern could still be successfully applied in ES6 mode.

I can make a pull request for:
Line 341 in emitter.ts

But it would be a learning curve to figure out how to write the appropriate tests in the format used by the compiler..

malibuzios

malibuzios Just curious, how do you specify your own __extends function to TS? Do you just define it in the global scope. Is there anyway to use it on specific classes?

The biggest take-away for me here was that using "target ES6" and "target ES5" results in totally different code behavior. If you develop in ES6 or target ES6 you cannot just switch to target ES5 if you want to target Internet Explorer for deployment. The best option you have is to avoid ES5 and then use babel, etc. to transpile to ES5. The way typescript implements ES5 inheritance is pretty much incompatible to the ES6 standard. The way statics are dealt with is one thing. But it also behaves differently with respect to "writable, enumerable, configurable, etc.". Switching your tsc compiler from one target to the other can easily break your projects and thus should be warned about in the documentation.

That said: "copying static members" to subtypes is a bad idea anyway, IMHO, and should be avoided totally in an ideal world. A common idiom is to have static "INSTANCE" fields for singletons and copying over those fields to subtypes is pretty stupid. Also copying over functions with side-effects can lead to very annoying bugs. Just my 2 cents.

This should be addressed by #12488

The behavior of __extends has changed with #12488. The changes will be part of the TypeScript 2.2. release. the new implementation uses setPrototypeOf when present making sure that static accessors are not copied by value.

Navigation Menu

Repository navigation

Expected behaviour of __extends with static accessors? #1520

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Expected behaviour of __extends with static accessors? #1520

Description

Activity

RyanCavanaugh commented on Dec 17, 2014

yortus commented on Dec 18, 2014

RyanCavanaugh commented on Dec 18, 2014

yortus commented on Dec 18, 2014

dcleao commented on Mar 8, 2015

malibuzios commented on May 6, 2016

Aryk commented on Dec 20, 2016

yGuy commented on Dec 28, 2016

mhegazy commented on Dec 28, 2016

mhegazy commented on Jan 3, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions