useDefineForClassFields breaks augmentation/overwrite of class members with decorators

[mzeiher]

TypeScript Version: 3.8.0-dev.20191113

Search Terms:
decorators, useDefineForClassFields, useDefineForClassFields decorators bug
Code

function Decorator() {
    return function (target, propKey, descriptor) {
        const symbol = Symbol();
        target.constructor.prototype[symbol] = descriptor ? typeof descriptor.initializer !== 'undefined' ? descriptor.initializer() : undefined : undefined;
        return {
            configurable: true,
            enumerable: true,
            set(value) {
                this[symbol] = value;
            },
            get() {
                return this[symbol] + 'decorated';
            }
        }
    }
}

class TestTSDefine {
    @Decorator()
    myProp = "test";
}

console.log(new TestTSDefine().myProp);

Expected behavior:
output should be 'testdecorated'

Actual behavior:
output is 'test'

Use the same code with "useDefineForClassFields = false"
output is the expected 'testdecorated'

with babel (also [[Define]] behaviour) everything works.

Playground Link: http://www.typescriptlang.org/play/?useDefineForClassFields=true&experimentalDecorators=true&ssl=1&ssc=1&pln=23&pc=40#code/GYVwdgxgLglg9mABAEQKYTgJwIZSwCgEpEBvAKEUsU1ShEyVElgUXym0wHNaAaRAA6Y4AgNKoAnvwAmqAM4RMMAXkzFyVTYgxg5URHIkBbAEZwANogC8iAMrGz5ogG4KWyh260AdDr2YQaCxvITg8KAkBVABtQ1MLAF1rRFkFJRUsRAB+RAiouGAU+UVlVW8YMBhYbHMYAC9UTEQAQisbAHJwWWAK1Gl27KK00uCKqpga+saiRAAuRC7UHrA+uYWwbt7pV3dKGjoGUjddqh0ernpsE3NUeagA1F5jk8RUMBAjRqubu4enl6oclo+AAbjUQKh1M8AR4ABYwOSxByJZJg8wQnYwgC+-xhPCgMw0MM0+3oSCg8MRcUcSQA1Ih2rIMDgoH12piAVjoVQuZouVyyBBzNg5HJEAAVeRQcW2NDLVBHTQAATQzNwBEIzyMEgACsIBMkAESsvSG1wCvwWVDecxwLj4FYAdwlUplct6RG82r1IkIziAA

** Repository to reproduce with Babel and Typescript **: https://github.com/mzeiher/decorator-ts-define-bug (see README)

[rictic]

Simpler example:

const Decorator: PropertyDecorator = () => {
    return {
        get() {
            return 'decorated value';
        }
    }
};

class TestTSDefine {
    @Decorator
    myProp = 'property initializer value';
}

console.log(new TestTSDefine().myProp);

http://www.typescriptlang.org/play/?useDefineForClassFields=true&experimentalDecorators=true#code/FAYw9gdgzgLgBAEQKbgE4EMZlQLjgBVTAAclUYBPZNTbOAXjgAoBKBgPjgG9g4+5USGAFdUEbr35SA5kNYSpi-oJFi4AcgAmKbJiSa4AN3QAbYUnUBuSUoC+NuPdvXQJ9FChwAKklheAysgAZgCWEEgK-AAC1LpYqA4AthSEJAxwAETERKTkFHBhITAhpiEAXmRGpuYZ1vagkFBgJkgAdCZg0kzhAO7evjABwWFIrK3JqcQs1kA

[mzeiher]

Regarding this bug, it would be nice if the typescript team could align the implementation for decorators and define semantics for fields to the current babel implementation (legacy mode) this would make a lot of things much easier especially for framework developers who ship decorator libraries (currently I'm detecting and testing TS legacy, Babel Legacy and the Babel Stage 2 implementation) a 4th decorator flavor makes it just harder to maintain (yes I know decorators are highly experimental ;))

one thing regarding inheritance, currently the useDefineForClassFields just overrides the property descriptor of super (defines a new one on the instance), it would be nicer if something like the babel helper could be used: this will just update the value but leave a possibly augmented (by a decorator or manual) intact (just the initalizer value is overridden)

function _defineProperty(obj, key, value) { if (key in obj) { Object.defineProperty(obj, key, { value: value, enumerable: true, configurable: true, writable: true }); } else { obj[key] = value; } return obj; }

[Kukkimonsuta]

In relation to replacing a class field/property/method - following code works on existing implementations of decorators with exception of TypeScript fields when using useDefineForClassFields. Unfortunately this is breaking without workaround. If fully aligning implementation with either babels "modern" or "legacy" decorators is too much work, perhaps consider implementing #29299 Option to disable decorator transform instead and let people further transform them themselves.

	Fields	Properties	Methods
Babel "modern"	✅	✅	✅
Babel "legacy"	✅	✅	✅
TypeScript (without "useDefineForClassFields")	✅	✅	✅
TypeScript (with "useDefineForClassFields")	⛔️	✅	✅

Code here:

Gist

TypeScript playground

Babel repl

[justinfagnani]

Ay updates here? We're increasingly seeing users complain that their components are broken because of this issue, and the symptom is very hard to track back to this unless you already know about it.

For some reason many tools that integrate the TypeScript compiler are enabling useDefineForClassFields and some of them don't give users control of the config, so there's no good workaround (except to delete own properties which puts objects into dictionary mode).

[brandon-marginalunit]

We're still seeing this in TypeScript 4.1.0-dev.20201020. It breaks the Aurelia framework, which is blocking us from using a library that requires useDefineForClassFields.

[Haprog]

I just ran into this because this seems to mean that it's not possible to use MobX decorators and lit-element decorators together with TypeScript. MobX decorators seem to require "useDefineForClassFields": true but that breaks lit-element decorators (like @property and @query).

[rbuckton]

This isn't a bug, but rather the expected behavior. It is unfortunate, but is a consequence of several interactions:

1. --useDefineForClassFields mimics the proposed behavior of ECMAScript fields.
2. The proposed behavior for ECMAScript fields is that they are defined in the constructor (i.e., using Object.defineProperty).
3. Defining a field on an instance means that a prototype walk will not occur, so the get/set you are defining on the prototype will never occur.
4. Defining a field does not trigger setters.
5. Even declaring a field like myProp: string; with --useDefineForClassFields will result in a new field definition on the instance that will shadow the prototype.

We introduced declare fields in TS specifically for this reason:

function Decorator() {
    return function (target, propKey, descriptor) {
        const symbol = Symbol();
        target.constructor.prototype[symbol] = descriptor ? typeof descriptor.initializer !== 'undefined' ? descriptor.initializer() : undefined : undefined;
        return {
            configurable: true,
            enumerable: true,
            set(value) {
                this[symbol] = value;
            },
            get() {
                return this[symbol] + 'decorated';
            }
        }
    }
}

class TestTSDefine {
    @Decorator()
    declare myProp: string; // does not result in redefinition
//  ^^^^^^^
    constructor() {
        this.myProp = "test"; // initializer must be moved to constructor
    };
}

console.log(new TestTSDefine().myProp);

Playground Link

[rbuckton]

Babel's decorators support permits two things TypeScript never has:

1. Decorating a "descriptor" for a field.
2. A decorator can intercept the initializer of a field descriptor.

These were part of an early draft for stage-2 decorators that has since been abandoned. Implementing this would be a major change for TypeScript decorators, a feature which is essentially "frozen" until a stable decorators proposal reaches Stage 3 in TC39.

Changing the behavior of decorators is out of scope, and changing the field definition semantics in --useDefineForClassFields to be incompatible with the ECMAScript fields proposal is also out of scope.

Until a stable decorators proposal reaches consensus, we don't intend to make any substantial changes to our decorators implementation. It may not be much, but we added declare fields as an alternative to further complicating the decorator landscape.

[justinfagnani]

The problem here is that some toolchains set this compiler flag and it breaks existing decorator usage. Previously, decorators could assume that they could define accessors and they would work. I think it would be reasonable to automatically use set semantics for decorated fields to keep them working even in the presence of this flag.