Compability of metadata structure with future extensions
Closed this issue · 8 comments
Currently when using .getMetadata/.setMetadata the values are associated not with the thing being decorated but the containing construct. However with the probable future extension of function/parameter decorators this makes it tricky to access relevant metadata on the object itself.
As an simple illustrative example, consider a docstring decorator that puts some help text in some metadata:
const DOC = Symbol("DOC");
function doc(docText) {
return function decorator(value, context) {
context.setMetadata(DOC, { kind: context.kind, docText });
}
}
class Lock {
@doc("Returns a promise that resolves when the lock is available")
acquire() {
// ...
}
}
@doc("Finds a prime between the given bounds")
function findPrime(
@doc("The minimum value for the prime") lowBound,
@doc("The maximum value for the prime") highBound,
) {
}As specified the only way to access the associated metadata for Lock.prototype.acquire is to read it off Lock.prototype[Symbol.metadata].public.acquire[DOC], whereas function decorators will presumably just have findPrime[Symbol.metadata][DOC].
Now one reason for the current behaviour is that in a class there can be multiple members with the same name, whereas for a function there is just one.
However I think it would be good to ensure that metadata can be accessed from the things being decorated themselves so that it's more compatible with future extensions. This would also be fairly handy for things like super.method[Symbol.metadata] rather than having to refer to the superclass directly.
There's a couple ways I can think of doing this:
- Just reflect class methods/accessor decorators onto the values themselves, so
Lock.prototype[Symbol.metadata].public.acquire[DOC]would be identical toLock.prototype.acquire[Symbol.metadata][DOC]- This particular method is addable later as it doesn't change any semantics of how metadata currently works
- Make all public metadata accessed as arrays when obtained from the class, instead calling
.setMetadatawould append a copy to the class array, while also directly setting the value for the value. Singular values could be accesssed by accessing those properties more directly.- e.g.
class Lock { @doc("Returns whether the lock is currently locked") get locked() { } @doc("Implemented for compability with older versions only, please don't use this") set locked() { } } Lock.prototype[Symbol.metadata].locked; // { [DOC]: [{ kind: "getter", docText: "...", ...] }
- My feeling that this is more consistent with the behaviour of how private fields are currently decorated so it shouldn't be too surprising
- Further decorators that are concerned only with decorating the current element will just receive the single value from
.getMetadata, this makes it quite easy to work on singular elements rather than trying to determine the "last set" metadata (especially painful in class heirarchies) - Do we actually have use cases where one would want to read off metadata fron previous class entries of the same named? Most that want to read bulk data would probably be a class decorator not a decorator on elements themselves
- If this sort've approach were to be taken it does need to be changed now, it probably can't be attached on later in any great way
Oh one more reason I think that doing metadata directly on elements, but adding to an array for class elements is that it would be very easy to accidentally create decorators that don't support get/set pairs, as both share the same name, if adding metadata naively with just .setMetadata one will lose the metadata for the first.
Having a separate methodOrAccessor[Symbol.metadata] and Class.prototype[Symbol.metadata] would make it a lot easier to write simpler decorators too, as with the status quo in order to handle all this stuff correctly one needs to keep track of a lot of information about what's currently being decorated, what's already been decorated, etc.
The primary reason that the metadata for class elements is accessed on the class itself is because not all class elements have a value which can be accessed directly. Specifically, class fields have no real value on the prototype which can be accessed at all, and getters and setters require users to read the methods with Object.getPropertyDescriptor which is very unergonomic.
The current API is uniform across all types of elements, and it is more ergonomic in general because of this as metadata based code can be the same for all types of class element decorators. Reflecting the metadata values onto methods and getters/setters specifically could be something added in the future for consistency with function decorators, if/when they are added, but it is unlikely it would be used much at the moment due to this inconsistency.
Make all public metadata accessed as arrays when obtained from the class, instead calling .setMetadata would append a copy to the class array, while also directly setting the value for the value. Singular values could be acesssed by accessing those properties more directly.
This type of Metadata API has been discussed a bit, and has some advantages in being simpler overall this is being discussed more in #424. However, I don't think it would be possible for setMetadata to simply append on each call, as there are use cases where decorators actually do want to read the metadata set by any previous decorators and either update it or do things like validations to ensure that the usage is valid.
Specifically, class fields have no real value on the prototype which can be accessed at all, and getters and setters require users to read the methods with
Object.getPropertyDescriptorwhich is very unergonomic.
Just to be clear I wasn't suggesting getting rid of class-level metadata at all, but rather .getMetadata/.setMetadata inside decorators concern the element it is decorating, not all elements of the same name.
However, I don't think it would be possible for
setMetadatato simply append on each call, as there are use cases where decorators actually do want to read the metadata set by any previous decorators and either update it or do things like validations to ensure that the usage is valid.
Similarly, yeah I underspecified this, basically if you decorate a single element, the final result of .setMetadata would be appended to the array i.e.:
const META_KEY = Symbol();
function logMeta(_, ctx) {
console.log(ctx.getMetadata(META_KEY));
}
function setMeta(value) {
return function(_, ctx) {
ctx.setMetadata(META_KEY, value);
}
}
class Foo {
@logMeta // Prints 2
@setMeta(2)
@logMeta // Prints 1
@setMeta(1)
@logMeta // Prints undefined, nothing set yet
get foo() {
}
@logMeta // Prints 20
@setMeta(20)
@logMeta // Prints 10
@setMeta(10)
@logMeta // Prints undefined, not affected by getter with same name
foo = 3;
}
// prints { public: { foo: [2, 20] } }
// this has the final values for each decorated item
console.log(Foo[Symbol.metadata]); Now if metadata does need to be shared across elements with the same name (this would mostly be useful for getter/setter pairs that work in tandem), then we could just have another method (e.g. ctx.getPreviousMetadata()):
const TYPE = Symbol("TYPE");
function typed(type) {
return function(value, ctx) {
if (ctx.kind === "getter") {
// Read the previous metadata for elements of the same name
const otherTypes = ctx.getPreviousMetadata();
// Find a corresponding setter which has a type
const setterMeta = otherTypes.find(({ kind }) => kind === "setter");
if (!type.assignableTo(setterMeta.type)) {
throw new TypeError(`getter must have subtype of setter`);
}
ctx.setMetadata(TYPE, { kind: "getter", type });
} else if (ctx.kind === "setter") {
// Same stuff as getter but backwards
const otherTypes = ctx.getPreviousMetadata();
const getterMeta = otherTypes.find(({ kind }) => kind === "getter");
if (!getterMeta.type.assignableTo(type)) {
throw new TypeError(`setter type must be a supertype of previous getter`);
}
ctx.setMetadata(TYPE, { kind: "setter", type });
}
// etc for other kinds
}
}
class Foo {
@typed(number)
get foo() {
}
@typed(Type.union(number, string))
@logTYPEMeta // Would print undefined, not set for this specific class-element
@logPreviousTYPEMeta // Would print [{ kind: "getter", type: number }]
set foo() {
}
}The current API is uniform across all types of elements, and it is more ergonomic in general because of this as metadata based code can be the same for all types of class element decorators.
Ergonomic is more of an opinion thing, I don't think it's ergonomic that using .setMetadata/.getMetadata on a setter will destroy previous metadata set on a getter with the same name (or vice versa). In order to avoid messing up previous values decorators need to do awkward things like shoving values into an array, or keeping some object with different kinds being decorated.
In fact that's why I think something like the the above suggestion of .getPreviousMetadata would be better, it keeps the distinction between "metadata on this element" vs "metadata on this class name" clear and means no awkward coordination of values is neccessary.
I believe that's how getMetadata is already supposed to work, unless I'm misunderstanding something about getPreviousMetadata.
I believe that's how
getMetadatais already supposed to work, unless I'm misunderstanding something aboutgetPreviousMetadata.
Not quite, .getMetadata/.setMetadata for public keys is essentially just a lookup table, if you call .setMetadata from what I tell from the spec you lose the previous value entirely.
Basically the pain points come up because get/set are paired you can easily blow away your previous data, but even if you are careful you need to move that metadata forward in a way all decorators decorating a particular element can follow.
i.e. From what I can tell from the spec this:
const DOC = Symbol("DOC");
function doc(docString) {
return function(_, ctx) {
ctx.setMetadata(DOC, docString);
}
}
class Point {
// ...
@doc("Gets the x coordinate of the point")
get x() {
return this.#x;
}
@doc("Sets the x coordinate of the point, for legacy reasons if a string is provided it will rounded to the nearest integer")
set x(newX) {
if (typeof newX === 'string') this.#x = parseInt(newX);
else this.#x = newX;
}
}
// Only prints the DOC for the setter
console.log(Point.prototype[Symbol.metadata].public.x);will print the doc for only one of the decorated elements, even though there are two.
Although from what you've said I think multiple decorators on the same decorated element is fine, this allows them to compose together. But this problem is really when there are multiple elements of the same name in a class, and in practice get/set are the only such elements of which there is significant reason to actually have multiple of the same name.
Because this issue is primarily a problem for get/set, it might be better to consider a solution for just that case. One thing I've been thinking of is similar to how get name()/set name() merge into a single descriptor, it would probably make symmetric sense for metadata on those elements to also merge.
i.e. It would be nice if the output of the above code was (actual doc strings omitted):
// { get: "<DOC for get x>", set: "<DOC for set x>" }
console.log(Point.prototype[Symbol.metadata].public.x);Although when working with .getMetadata/.setMetadata on one of these accessors, we would still want it to deal with the values directly, i.e. .getMetadata() when decorating get x() would still return "<DOC for get>".
Now for this to work, there's a few ways I think it could be done, but a simple way I think would be that the metadata list contains separate entries for getters and setters, so the public metadata list would contain entries like:
[
{ prop: "x", isAccessor: true, value: {
get: "<DOC for get x>",
set: "<DOC for set x>",
},
{ prop: "toDist", isAccessor: false, value: "<DOC for method toDist>" },
]Effectively .setMetadata would be changed so that step 9 is (and similarly for private metadata):
9. If kind is public, then
a. Let publicMetadata be the element of metadataForKey.[[Public]] whose [[Key]] is the same as propertyKey.
b. If publicMetadata does not exist, then
i. If accessorKind is "get", then
a. Set publicMetadata to the Record { [[Key]]: propertyKey, [[IsAccessor]]: true, [[Value]]: Record { [[GetterValue]]: value, [[SetterValue]]: undefined } }
ii. Else, if accessorKind is "set", then
a. Set publicMetadata to the Record { [[Key]]: propertyKey, [[IsAccessor]]: true, [[Value]]: Record { [[SetterValue]]: value, [[SetterValue]]: undefined } }
iii. Else
a. Set publicMetdata to the Record { [[Key]]: propertyKey, [[IsAccessor]]: false, [[Value]]: value }
iv. Append publicMetadata to metadataForKey.[[Public]].
c. Else
i. If accessorKind is "get", then
a. If publicMetadata.[[IsAccessor]] is true, then
i. Set publicMetadata.[[Value]].[[GetterValue]] to value
b. Else
i. Set publicMetadata.[[IsAccessor]] to true
ii. Set publicMetadata.[[Value]] to the Record { [[GetterValue]]: value, [[SetterValue]]: undefined }
ii. Else, if accessorKind is "set", then
a. If publicMetadata.[[IsAccessor]] is true, then
i. Set publicMetadata.[[Value]].[[SetterValue]] to value
b. Else
i. Set publicMetadata.[[IsAccessor]] to true
ii. Set publicMetadata.[[Value]] to the Record { [[GetterValue]]: undefined, [[SetterValue]]: value }
iii. Else,
a. Assert accessorKind is undefined
b. Set publicMetadata.[[IsAccessor]] to false
c. Set publicMetadata.[[Value]] to value
Or another to see this would be to consider the following set of operations:
const KEY = Symbol("KEY");
function setMeta(value) {
return function(_, ctx) {
ctx.setMetadata(KEY, value);
}
}
class Point {
// Before: { public: {} }
// This decoration set CREATES a get metadata value
// AFTER: { public: { x: { get: 1, set: undefined } } }
@setMeta(1)
get x() {
}
// This decoration AUGMENTS the existing metadata
// AFTER: { public: { x: { get: 1, set: 2 } } }
@setMeta(2)
set x(x) {
}
// This decoration REPLACES the existing get metadata value
// AFTER: { public: { x: { get: 3, set: 2 } } }
@setMeta(3)
get x() {
}
// This decoration REPLACES the existing metadata with a plain value
// as the decorated element is a method and a method will destroy
// an accessor pair on a class
// AFTER: { public: { x: 4 } }
@setMeta(4)
x() {
}
// This decoration REPLACES the existing metadata as we have a new accessor
// replacing a method, note that the old value of set is gone, this corresponds
// to the fact that the set accessor itself it also gone, i.e. the metadata
// mirrors the shape of the class
// AFTER: { public: { x: { get: 5, set: undefined } }
@setMeta(5)
get x() {
}
}The nice thing about this metadata format merging is the metadata object structurally matches the actual values a lot more closely, however the main thing that is weird is how would .getMetadata behave inside such a decorator. My expectation would be that ctx.setMetadata(3); ctx.getMetadata() would return 3 as it was the value just set, but if this is the case there's no way for a setter/getter to get each other's metadata, so for these cases I'd propose another method .getAllMetadata which returns the same value as would be stored in Point.prototype[Symbol.metadata].x, i.e. either a value from a previous method, or a get/set record.
I see where you're coming from, but it does add a lot of complexity to the design and I'm not sure if that complexity makes sense. In our surveys of the community and experience from a large number of framework and decorator authors, decorators which act on both get and set are actually very uncommon, which is why making them decorate-able as separate elements was an acceptable change in the first place. To be fair, it's also not unheard of, but that's what the Grouped and Auto-Accesors proposal is about - allowing decorators to decorate groups of getters and setters altogether, so you don't run into issues like this.
In the meantime, this API gives authors an escape hatch that they can use to manually combine metadata in cases where it is important for values which have the same name are effectively decorated together. Outside of get and set, this is really very uncommon, so I'm not sure we should prioritize a change just for that use case.
decorators which act on both
getandsetare actually very uncommon
Note that because get/set decorators both just receive functions as the first argument, the majority of function-wrapping decorators (i.e. things like @lruCache({ size: 10 }), @logCall(logger), @assertReturn(number)) will just work with get/set. So I don't think it'll be that uncommon. The previous iterations of decorator proposals that are implemented in transpilers was really quite different that people would only adopt them if there was a library that used them sufficiently much to be worth it.
And this leads to a pretty big hazard, if a decorator was only really considered for methods, but people are using it on getters/setters, a library would likely use .getMetadata/.setMetadata considering only the "method" (or perhaps function in future) use and inadvertently break the metadata for uses of get/set, even though previously those uses were perfectly reasonable to assume would work. (In even the best case the metadata is harmless and the last one simply wins (probably setter), but this is fairly useless).
To be fair, it's also not unheard of, but that's what the Grouped and Auto-Accesors proposal is about - allowing decorators to decorate groups of getters and setters altogether, so you don't run into issues like this.
And unfortunately I don't think the grouped accessors proposal would help super much with this hazard, unless you can decorate the get/set inside the grouped accesor (which would essentially just be function decorators). If decorating inside grouped accesors isn't allowed, then it just removes the ability to use a bunch of function-wrapping decorators which otherwise work fine.
And I can personally say that once decorators land in the actual language, I for one will be using wrapping-decorators for everything and then some more. As I already use accessors in basically every class already, I expect to using decorators like:
class Point {
@doc("Returns the x coordinate of the point")
@assertReturnType(number)
get x() {
}
@doc("Sets the x coordinate of the point")
@assertParamTypes([number])
set x(x) {
}
@doc("Returns the distance to the given point")
@assertParamsTypes([Point])
@assertReturnType(number)
dist(point=Point.origin) {
}
}on basically every single class I write from that point forward. Really the main reason I'm not doing this already is because the decorator proposal has been as unstable as it has over the years, so I'm not really willing to commit all-in on some transpiler's implementation.
I really doubt I'm the only developer in this camp, so I wouldn't expect heavy degrees of decorator usage to be that uncommon, including decorating getters and setters.
I see where you're coming from, but it does add a lot of complexity to the design and I'm not sure if that complexity makes sense.
In some regards, although I feel the fact .getMetadata/.setMetadata work the same when decorating get/set as when decorating other class elements would be less complex for most people. Similarly I think { public: { x: { get: { ... }, set: { ... } } } would also be less complex for most developers looking at this, at it more naturally matches what they wrote in the class, i.e. for a get in the class there is a get in the metadata.
The only case that the status quo is less complicated for is for people who are sharing metadata across elements with the same name, which itself at current only really makes large sense on get/set anyway, so if as you say decorators specifically targeting get/set (ignoring function-wrapping I mentioned above), then this seems like a fairly minor reason to keep the status quo.
As noted above, it is possible to write decorators which share metadata within the current structure, and the current structure is also extensible for future extensions. In addition, an alternative simpler metadata structure has been proposed in #451. As such, I'm going to close this issue.