jayphelps / core-decorators

Library of stage-0 JavaScript decorators (aka ES2016/ES7 decorators but not accurate) inspired by languages that come with built-ins like @​override, @​deprecate, @​autobind, @​mixin and more. Popular with React/Angular, but is framework agnostic.
MIT License
4.51k stars 263 forks source link

@nonenumerable makes property readonly in typescript using strict #78

Open janjilek opened 8 years ago

janjilek commented 8 years ago

Hi, then I try to use nonenumerable decorator, I cannot write to the variable. I receive this exception: Cannot assign to read only property. Is this expected behavior?

jayphelps commented 8 years ago

@janjilek can you please provide reproducible example code?

timofei-iatsenko commented 8 years ago

Same issue.

Example is simple:

class Foo {
  @nonenumerable
  property = {
    foo: 1,
    bar: 1,
  }
}

new Foo();

In console:

Uncaught TypeError: Cannot assign to read only property 'property' of object '#'

Typescript Compiler 1.8.10

timofei-iatsenko commented 8 years ago

May be it will be helpfull, generated code is

"use strict";

var _typeof = typeof Symbol === "function" && typeof Symbol.iterator === "symbol" ? function (obj) { return typeof obj; } : function (obj) { return obj && typeof Symbol === "function" && obj.constructor === Symbol ? "symbol" : typeof obj; };

function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

var __decorate = undefined && undefined.__decorate || function (decorators, target, key, desc) {
    var c = arguments.length,
        r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc,
        d;
    if ((typeof Reflect === "undefined" ? "undefined" : _typeof(Reflect)) === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc);else for (var i = decorators.length - 1; i >= 0; i--) {
        if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
    }return c > 3 && r && Object.defineProperty(target, key, r), r;
};
var __metadata = undefined && undefined.__metadata || function (k, v) {
    if ((typeof Reflect === "undefined" ? "undefined" : _typeof(Reflect)) === "object" && typeof Reflect.metadata === "function") return Reflect.metadata(k, v);
};
var core_decorators_1 = require('core-decorators');

var Foo = function Foo() {
    _classCallCheck(this, Foo);

    this.property = {
        foo: 1,
        bar: 1
    };
};

__decorate([core_decorators_1.nonenumerable, __metadata('design:type', Object)], Foo.prototype, "property", void 0);
exports.Foo = Foo;
jayphelps commented 8 years ago

AFAICT the issue is that TypeScript is not generating an initial descriptor that all our decorators expect is being provided.

So what happens is, the descriptor ends up just looking just like this:

{ enumerable: false }

Which means the defaults of Object.defineProperty will be used, including writable: false.

This behavior is contrary to the behavior Babel v5 had, as well as babel-plugin-transform-decorators-legacy and again AFAICT contrary to every iteration of the decorators spec.

This appears at first glance to be a deviation from the proposed decorators spec and I'm not sure I can work around this. 😢

jayphelps commented 8 years ago

This appears to be a known issue for property initializers only, they're choosing not to address at the moment:

NOTE  A Property Descriptor is not provided as an argument to a property decorator due to how property decorators are initialized in TypeScript. This is because there is currently no mechanism to describe an instance property when defining members of a prototype, and no way to observe or modify the initializer for a property. As such, a property decorator can only be used to observe that a property of a specific name has been declared for a class.

http://www.typescriptlang.org/docs/handbook/decorators.html#property-decorators

The solution (right or wrong) that Babel took was providing a default descriptor.

e.g.

(TEMP = Object.getOwnPropertyDescriptor(TARGET, PROPERTY), (TEMP = TEMP ? TEMP.value : undefined), {
    enumerable: true,
    configurable: true,
    writable: true,
    initializer: function(){
        return TEMP;
    }
})

All of this can be attributed to the fact that the decorators spec is very much non-trivial to implement. The spec was and still is rough and ambiguous at this stage, so some transpilers will choose to fill in the gaps with assumptions while others will refuse.

reinert commented 7 years ago

I'm running into this issue too. We have to explicitly set writable = true in the descriptor so we can write to the property while keeping it non enumerable. It would be useful to have a @writable decorator or, even better, an option to set writable=true in the @nonenumerable decorator. (something like @nonenumerable(writable=true))

reinert commented 7 years ago

I proposed a patch where @nonenumerable sets writable=true, unles the user specifies @nonenumerable({ writable: false })

jayphelps commented 7 years ago

@reinert hmm not sure of the solution you're proposing but if it's easy enough could you submit a PR with tests?

ghost commented 7 years ago

Just got stuck on the same issue 😕 Any progress on this?

timofei-iatsenko commented 7 years ago

TS implementation of decorators different from babel's, so there can't be any progress. This library designed for ES decorators.

By the way, if you need @nonenumerable decorator in typescript, you can pick my version:

export function nonenumerable(target: any, key: string) {
  // first property defined in prototype, that's why we use getters/setters
  // (otherwise assignment in object will override property in prototype)
  Object.defineProperty(target, key, {
    get: function() {
      return undefined;
    },
    set: function(this: any, val) {
      // here we have reference to instance and can set property directly to it
      Object.defineProperty(this, key, {
        value: val,
        writable: true,
        enumerable: false,
      });
    },

    enumerable: false,
  });
}
ghost commented 7 years ago

@thekip Thanks! Way more robust than the one I was going for.

endel commented 7 years ago

@thekip works great! I've extracted your implementation into a standalone module here: https://github.com/endel/nonenumerable