wycats / javascript-decorators

2.4k stars 127 forks source link

Property initializers #71

Open fatfisz opened 8 years ago

fatfisz commented 8 years ago

Continuing from https://phabricator.babeljs.io/T2005 and #41 (I don't recommend reading the latter issue though).

Currently property decorators for both classes and objects operate on a descriptor pseudo-property "initializer", based on this document. In my opinion this is over-complicating and also misleading in the case of class properties, because:


In the first referred issue @wycats says:

It's a critical constraint that decorators can run only once when the class is built and not add additional runtime cost unless the decorator chooses to add cost (by wrapping, for example). You can achieve both goals via initializer functions, but deferring the decorator until instantiation adds unavoidable cost.

So there is a performance concern. But still, for me the least surprise is more important. If I wanted to write really performant code, I wouldn't use decorators at all - they make life easier only for the programmer and not for the machine that will be executing the code.

@isiahmeadows @jayphelps @jeffmo @loganfsmyth @lukescott Could you please weigh in on this?

lukescott commented 8 years ago

The issue revolves around prototype vs instance. Class properties are bound to the instance. Class methods are bound to the prototype. That's why class properties need an initializer. Even without exposing the initializer on the descriptor for the benefit of decorators (and possibly other uses) this would still be the case.

Class property or method you could add/remove an initializer. The initializer is only not used for static properties or methods.

There really isn't much you can do about this. It seems silly for value types. But with reference types it would be more surprising for multiple instances to be sharing the same object.

fatfisz commented 8 years ago

@lukescott I'm not against the initializer functions at all, they are necessary for the class fields. I just think that the descriptor pseudo-property "initializer" used for decorators is over-complicating, because decorators have to handle two symmetric cases depending on whether they are applied to a property (field) or to a method.

I suggest that the descriptors for the plain object properties receive "normal" descriptors, because that's more intuitive.

And for the class fields I suggest the decorators are applied after the initializers are invoked in the constructor. Decorators would be invoked every time the object is constructed, but it wouldn't be any different from how initalizers behave now.

silkentrance commented 8 years ago

@fatfisz Same issue here, we are unable to properly decorate the initialized instance property https://github.com/jayphelps/core-decorators.js/issues/62#issuecomment-211030001

Moving the decoration process into the constructor along with actually defining the instance property is one option. I for my part would like to see that property defined on the prototype, along with all the others instead of defining it every time that the class is instantiated.

jayphelps commented 8 years ago

I'm not seeing an alternative that solves the issue of prototype vs. instance (as lukescott touched on); IMO this is a hard-requirement. Perhaps I'm missing it, can someone provide code of an alternative?

fatfisz commented 8 years ago

@jayphelps @silkentrance Maybe I didn't fully convey what I had in mind, so please bear with me.

I'm suggesting that something like this:

class Person {
  @readonly
  born = Date.now();
}

should be desugared roughly to:

class Person {
  constructor() {

    // Iterate over all class fields declared on the prototype
    for (const [key, initializer] of Person.prototype[Symbol.ClassPropertyStore].entries()) {

      // Initialize the descriptor
      let descriptor = {
        value: initializer.call(this),
        enumerable: true,
        writable: true,
        configurable: false
      };

      // Apply the decorators - the order should be reversed
      const decorators = Person.prototype[Symbol.ClassPropertyDecoratorsStore][key] || [];
      for (let index = decorators.length - 1; index >= 0; index -= 1) {
        descriptor = decorators[index](this, key, descriptor) || descriptor;
      });

      // Finish by setting the descriptor on the instance
      Object.defineProperty(this, key, descriptor);
    }
  }
}

// Initializer declarations
Person.prototype[Symbol.ClassPropertyStore] = {
  born() {
    return Date.now();
  }
};

// Decorator declarations
Person.prototype[Symbol.ClassPropertyDecoratorsStore] = {
  born: [readonly]
};

I used symbols only for the sake of an example. IIRC the field initalizers are to be declared on a private slot, later accessible through some reflection method. Symbol.ClassPropertyDecoratorsStore is an addition of mine; I think that if the decorators were to be applied each time in the constructor, they should also be stored in the same way as the class fields.

So, to sum up:

silkentrance commented 8 years ago

@fatfisz makes sense, however, decoration at the instance level requires an altogether different protocol and will complicate things rather than simplify them, as now you have to check whether you are decorating a function/class or prototype or even an instance thereof.

I for my part would still vote for moving the property declaration process and assignment of the value returned by the initializer function out of the constructor. That way, of course, one cannot use Date.now() anymore as it will be the same value for all instances of that class. And I personally also believe that it is kind of a bad architecture to do so. Other examples would be to associate a UUID with the instance and so on.

Other use cases, for example calling upon a, preferably constructor injected, service cannot be captured by initialized properties at all and one must call these explicitly in the constructor. (not the injection of course, but calling upon other services)

And since that would be the major use case, I don't think that initialized instance properties should be defined in the constructor but rather during class construction and on the prototype.

Just read this in the official spec Let initializerResult be the result of evaluating fieldInitializerExpression with this equal to instance. So my former argument might be moot...

And, considering the following problem that (decorated) initialized instance properties currently expose

class Base
{
   foo = '5';
}

class Derived extends Base
{
    foo() {return 'foo';}
}

Now, at design time everything works as expected but when instantiating Derived, the properties from the base class will shadow those defined by the derived class, and only when instantiating the class. So there are other issues with those initialized instance properties that need to be solved first and the only way I see that this can be accomplished is by moving the property definition out of the constructor.

new Derived().foo();
              ^

TypeError: (intermediate value).foo is not a function

See also https://phabricator.babeljs.io/T7301

silkentrance commented 8 years ago

@fatfisz And I think that we should/need to distinguish between class/instance here.

Initialized properties, as they are defined now, can be static

class Foo
{
   static bar = 1;
}

or they can be declared on the instance

class Foo
{
   bar = 1;
}

So, the first one I would call either an initialized static property or rather an initialized class property., as it is defined on the class. The latter I would refer to as an initialized instance property as it is defined on the instance and only when instantiating the class.

silkentrance commented 8 years ago

See also https://github.com/jeffmo/es-class-fields-and-static-properties

fatfisz commented 8 years ago

@silkentrance My issue is based on how things are now, and currently the initialization happens inside the constructor. Decorating properties there is akin to decorating properties of a simple object, only this is a special one - this. Decorating simple object properties is an existing use case, so there shouldn't be a need to specifically cater for it.

In any case, let's see how jeffmo/es-class-fields-and-static-properties#38 turns out, because the problem you highlighted is an interesting one. One thing should be noted, this was already possible with simple functions, so it isn't something inherent to the new class syntax:

function Base() {
  this.foo = '5';
}

function Derived() {
  Base.call(this);
}
Derived.prototype.__proto__ = Base.prototype;
Derived.prototype.foo = function () { return 'foo'; };

new Derived().foo(); // throws an error
lukescott commented 8 years ago

@silkentrance Just want to make sure I understand you. Are you saying that this:

class Foo {
  bar = "bar";
}

Should be:

class Foo {

}
Foo.prototype.bar = "bar";

Instead of:

Should be:
class Foo {
  constructor() {
    this.bar = "bar";
  }
}

?

If so, this would be a huge problem:

class Foo {
  constructor() {

  }
}
Foo.prototype.bar = {value:"bar"}; // object is shared!

Hence the reason why it is the way it is now.

If not, can you clarify?

silkentrance commented 8 years ago

@lukescott yes, this is what I was saying and proposing. I reckon that this could be a 'major' problem with existing frameworks. But this is not to be discussed here, sorry for coming up with that in the first place, but I feel that this needs to be addressed, especially since this behavior does introduce some special casing on the behavior of how decorators are being applied and how they can be applied. Especially when considering initialized instance properties.

Otherwise, and as was proposed by @fatfisz, the decoration process needs to be moved to the instance as well, which again exposes some very specific difficulties in determining whether the decorator is applied at design time or during runtime.

Thus my counter proposal over at https://github.com/jeffmo/es-class-fields-and-static-properties/issues/38#issuecomment-212836508 which declares initialized fields as standard fields, having both a getter and setter and thus need not be decorated during runtime and also solving the above mentioned problem with said frameworks. Of course, this would require some convention or altogether disallow the user from accessing the internal and thus private state of the property other than by the specified getter or setter.

silkentrance commented 8 years ago

@fatfisz Hey, please have a look at the new spec proposal over at https://github.com/tc39/proposal-decorators. Is it just my failing eyes or have class properties been eliminated?

fatfisz commented 8 years ago

@silkentrance Maybe the new spec only is concerned with decorators in isolation from any other features in making? It's hard to tell what could become of decorators now, even their expected signature is missing from the spec. What decorating class fields will look like could be defined much later.

Edit: For now I'm waiting for some updates here: https://github.com/tc39/tc39-notes.

dead-claudia commented 8 years ago

@fatfisz I wouldn't be surprised if that were the case. Plus, once decorators exist for methods, it's not hard to extend for property initializers. But if they limit it to existing, known ES7 features, it's much easier to work on it, as there are 0 dependencies. I noticed a similar thing happen with SIMD types - they're literally going to be a new unboxed primitive, and they were initially thought of as a new value type, but value types didn't exactly pan out very quickly, so it was separated from that strawman very early on.

icodeforlove commented 7 years ago

This just bit me.

My assumption was that

class Example {
  property = false
}

Was the same as

class Example {
}
Example.prototype.property = false;

But to my surprise its actually

class Example {
  constructor() {
    this.property = false
  }
}

This is VERY odd, and I'm not sure why we can't just place them on the prototype.

With the current implementation it seems impossible to make class level instance/prototype decorators work.

mweststrate commented 7 years ago

@icodeforlove because

class Example {
   property = { hi: "hello" }
}

would result in the object being shared by all instances. So exampleInstance.property.hi = "Bye" would actually "modify" all instances of Example

What you want is:

class Example {
  static property = false
}
icodeforlove commented 7 years ago

@mweststrate I always assumed it was shared. If I didn't want it to be shared I would have placed it in the constructor.

The argument that objects should not be shared seems ridiculous to me, lets look at the current method implementation

class Example {
    constructor () {
        console.log('shared: ', this.method.shared);
        this.method.shared = true;
    }

    method () {
    }
}

new Example(); // logs out 'shared: false' on first initialization
new Example(); // logs out 'shared: true' on every subsequent initialization

Methods seem pretty shared to me, what is the mental jump to assume that everything is shared unless inside of a constructor?

I would agree if methods are not shared, then this implementation could make sense. But we wouldn't do that because of initialization costs.


So right now we can't do the following?

@decorator
class Example {
  // from what i understand theres no way to see this prop other than parsing the constructor 
 property = false
}

We can do this

@decorator
class Example {
 static property = false
}

We also can do this

class Example {
 @decorator
 property = false
}

But we wont get a descriptor for the property :/

At the moment it seems best to stay away from decorating the transform-class-properties, vs explaining the oddities which seem like they will probably change soon.