-
Notifications
You must be signed in to change notification settings - Fork 127
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
Property initializers #71
Comments
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. |
@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. |
@fatfisz Same issue here, we are unable to properly decorate the initialized instance property jayphelps/core-decorators#62 (comment) 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. |
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? |
@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. So, to sum up:
|
@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 And, considering the following problem that (decorated) initialized instance properties currently expose
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.
See also https://phabricator.babeljs.io/T7301 |
@fatfisz And I think that we should/need to distinguish between class/instance here. Initialized properties, as they are defined now, can be static
or they can be declared on the instance
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 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 - In any case, let's see how tc39/proposal-class-public-fields#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 |
@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? |
@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 tc39/proposal-class-public-fields#38 (comment) 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. |
@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? |
@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: |
@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. |
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. |
@icodeforlove because
would result in the object being shared by all instances. So What you want is:
|
@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 |
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:
This descriptor property is not used anywhere else; it won't even be found in the class fields and static properties spec - initializers for the class fields are kept as simple functions (not descriptors).
It differentiates between the properties and the methods, e.g.:
and
foo
has to handle two different versions of decorators for these cases.The "initializer" property exists because of class properties, which are sugar for initializing instance properties in the constructor. I think it is misleading that the decorators are applied only once, whereas the initializers are called each time the class is instantiated.
In the first referred issue @wycats says:
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?
The text was updated successfully, but these errors were encountered: