'kind' value is 'method' on decorator descriptor with get/set property definition #162
Comments
|
Fields are own properties on the instance, that's a getter/setter pair on the prototype - i agree that calling it a "method" is a bit confusing, but it's definitely not a field. I wonder if we could change the "kind" to be something like "accessor"? |
|
placement: 'own' and placement: 'prototype' are the right way for identify when the property is defined into the prototype or into the object. It's not necessary define other kind. |
|
That's true, but there is a difference between a field (which has a per-instance initializer), a method (a function), a data property, and an accessor property, which doesn't seem currently captured by the "kind". |
|
You can use the descriptor for check if a get/set or a value is defined for this property, but is a indirect route. Off course, I can check the kind is 'method' and descriptor has a get or set function. Perhaps the 'accessor' kind is a good solution. |
|
FWIW a much earlier draft from @wycats used |
|
It's always possible to identify the kind with the descriptor. For example, a method can be checked with Similarly, we can identify a get / set with checking
|
|
@pabloalmunia Can you tell me about a case where you want to make this distinction from inside a decorator? |
|
@littledan It is a little complicated to include a real example here. We are building an extension framework based on decorators with more than 10,000 lines of code. We have prepared this simple case step by step. Step 1.- Simple case: function intercept(desc) {
if (desc.kind === 'method') {
const originFn = desc.descriptor.value;
desc.descriptor.value = (...args) => {
console.log(`call ${desc.key} with this parameters`, ...args);
return originFn(...args);
};
} else {
// field
}
return desc;
}
class Demo {
@intercept
x (a, b) {
return a + b;
}
@intercept
z = 10;
}
const d = new Demo();
console.log(d.x(10, 20));Work without problem and intercept the method call and ignore the property. Step 2.- Add get / set without change into the decorator: class Demo {
@intercept
x (a, b) {
return a + b;
}
@intercept
get w() {
// ...
}
set w(v) {
// ...
}
@intercept
z = 10;
}The code is broken because the descriptor is wrong when the case is a get/set accessor: Step 3.- Add support for get / set accessor: function intercept(desc) {
if (desc.kind === 'method') {
if (desc.descriptor.get) {
const originalGet = desc.descriptor.get;
desc.descriptor.get = () => {
console.log(`get ${desc.key}`);
return originalGet();
};
if (desc.descriptor.set) {
const originalSet = desc.descriptor.set;
desc.descriptor.set = (v) => {
console.log (`set ${desc.key} with value`, v);
return originalSet ();
};
}
} else {
const originFn = desc.descriptor.value;
desc.descriptor.value = (...args) => {
console.log(`call ${desc.key} with this parameters`, ...args);
return originFn(...args);
};
}
} else {
// field
}
return desc;
}
class Demo {
@intercept
x (a, b) {
return a + b;
}
@intercept
get w() {
// ...
}
set w(v) {
// ...
}
// @intercept
// z = 10;
}
const d = new Demo();
console.log(d.x(10, 20));
d.w = 20;
const n = d.w;In our real code the problem is more complicated, and perhaps this example is an extreme simplicity, but we need to differentiate the methods of the access descriptor with get / set in many situations. Kind regards. |
|
I would be fine with this change, but I'm still having trouble understanding the complexity. The logic in the specification would be the equivalent of the JS code, |
|
Yes, it is an elegant solution. As a result, We appreciate your attention and we are very grateful for the new decorator specification. |
|
Thanks! I'd love to hear more about your project. |
|
I assume that in the case of getters and setters, in most cases you'd only need to decorate whichever appears first? Also, I'm wondering if three categories for the |
|
The current distinction is that |
|
@mbrowne matching getter/setter pairs should be coalesced before the decorator is called. A field is certainly a property with an initializer; a method, to me, is a function-valued data property on the prototype. |
|
Of course, strictly speaking, you can infer what type of element is decorated inspecting the descriptor without consulting the kind property. You can check if the descriptor has the get / set methods (accessor) , if the value property is assigned to a function (method) and in any other case it is a property (field). I understand that |
|
Sorry, it's close by error. |
|
Nicolo explains it well #162 (comment) . I would add, methods are added before fields. You can actually make a decorator which creates a method whose value is not a function, and things like that, FWIW, but there is no syntax for that. |
|
Just to specify, I'm not opposed to the |
|
@nicolo-ribaudo It's true, the method |
|
Another consideration is that if an "accessor" kind were added, the return value in decorator functions would have yet another way of specifying the same thing. If I recall correctly it's already possible to return an element descriptor that creates an accessor property with |
|
@mbrowne it's true, now the conversion from field to get/set with a decorators is this: function check (descriptor) {
if (descriptor.kind === 'field') {
return {
kind : 'method',
key : descriptor.key,
placement : descriptor.placement,
descriptor : {
get () {
console.log ('get', descriptor.key);
},
set (v) {
console.log ('set', descriptor.key);
}
}
}
}
return descriptor;
}
class Test {
@check
z = 10;
}
const d = new Test ();
d.z = 20;
let r = d.z; |
|
@mbrowne Currently there is only one way (using "method"). If you return a "field" with a get/set descriptor, it throws (step 16 of https://tc39.github.io/proposal-decorators/#sec-to-element-descriptor) |
|
There's a higher level issue here, which is that the decorator API isn't very developer-friendly in general. It is easy to use decorators and hard to write your own. We should consider publishing a utility library to make this easier, and checking whether a descriptor is an accessor could be part of that. |
|
Sorry for my incorrect comment above...I guess it underscores the point that the API could be more intuitive. One significant confusing thing about it is that it includes both the new concept of class/element descriptors and also ES5 property descriptors. While ES5 property descriptors have the benefit of familiarity and being closer to the metal, I still wonder if they need to be included in the public API at all or if they could be abstracted as an implementation detail. If a nested object were only needed to specify enumurable/writable/configurable, that would be fine. But when you get into initializer vs. property descriptor value, and which |
|
For internal use, we are build two simple tables with the decorator descriptor parameter and return with some note when the behavior need explain.
1 2 3 when 4 when the decorator is apply over a getter/setter 5 when 12
6 7 8 9 10 cannot include 11 |
|
@mbrowne Do you have any particular ideas for how it could be changed? I don't see a clear way to "abstract" things without making it worse in some way or other, but maybe you do. |
|
@pabloalmunia Thanks, that's helpful. Could we add this table to the documentation for this proposal? |
Off course. Please, review our contribution, we don't are expert in this kind of code, only an enthusiastic team with this new feature. For example, we don't kwon the behavior with private #method an #fields and it's not include in this table. |
|
FWIW the behavior for private is enumerated in TAXONOMY.md. |
Thks. We update our tables with some little fixed after read this explain. |
|
@pabloalmunia Any way you could submit the table as a PR? |
|
@littledan I think it might help to lift the property descriptor properties up to the top level, e.g.: {
key: 'foo',
kind: 'method',
placement: 'prototype',
enumerable: false,
writable: true,
configurable: true,
value: () => { console.log('foo') }
}I think that the common expectation will be that you can use property descriptors like you would with But I don't want to overstate this criticism. Overall I don't think the current API is bad and I think the learning curve is pretty small. I'm just saying that the issues I mentioned above will probably trip some people up. |
|
@mbrowne this is a good approach, we have a lot of confusions talking about decorator descriptor than includes a property descriptor. In the same way, and in my humble opinion the use of |
Done... |
|
I've been thinking about this more, and I'm increasingly convinced that it makes sense to use |
|
OK, we've adopted |
I'm not sure, but I understand that when we decorate a property with get and set methods the descriptor that is received in the decorator should be of kind 'field' and the get and set method are included into de property descriptor. Now the kind value is 'method' as is inferred from:
This a very simple example with Babel 7.1:
The result is:
In my humble opinion the kind property of the last case must be 'field'.
I apologize in advance if I am wrong.
The text was updated successfully, but these errors were encountered: