-
Notifications
You must be signed in to change notification settings - Fork 106
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
Data sharing across decorators #328
Comments
Btw, it's possible to implement something close in userland, but it requires explicit opt-in, and wouldn't be as optimized. function Placeholder() {
const placeholder = {};
const initializers = [];
const extended = function(decorator) {
return function(...args) {
return decorator.call(this, ...args, placeholder);
};
};
const init = function(decorator) {
return function(klass, {type}) {
if (type !== "class") throw new Error('Unexpected type. Please use decorator on "class"');
initializers.push(decorator(undefined, {type: "initializer"}, placeholder));
return klass;
};
};
const applyInit = function(klass, {type}) {
if (type !== "class") throw new Error('Unexpected type. Please use decorator on "class"');
return class extends klass {
constructor(...args) {
super(...args);
initializers.forEach(initializer => initializer.call(this));
}
};
}
return {extended, init, applyInit);
}
const boundNames = Symbol("boundNames");
const bound = (method, {name}, placeholder) => {
let boundSet = placeholder[boundNames];
if (!boundSet) boundSet = placeholder[boundNames] = new Set();
boundSet.add(name);
return method;
};
bound.init = (_, context, placeholder) => {
return function() {
const boundSet = placeholder[boundNames];
if (!boundSet) return;
for (const name of boundSet) {
this[name] = this[name].bind(this);
}
};
};
const {init, applyInit, extended} = Placeholder();
@applyInit
@init(bound.init)
class C {
#x = 1;
@extended(bound) method() { return this.#x; }
}
let c = new C;
let m = c.method;
m(); // 1, not TypeError With built-in support, the idea would be to have the "initializer" decorator type be applied to the constructor instead of the class as above. This might not be ideal as it'd require the constructor to be explicit (the default constructor cannot be used since there would be nothing to attach the decorator to). class C {
#x = 1;
@bound.init constructor() {}
@bound method() { return this.#x; }
} We could also imagine ordering the "initializer" decorators before others so that they can setup the placeholder. That would allow decorators that rely on an initializer to throw if the initializer decorator wasn't added to the class. |
Here is how the const visibilityPrivates = new WeakMap();
const visible = (accessors, context, placeholder) => {
let visibilityGetters = visibilityPrivates.get(placeholder);
if (!visibilityGetters) boundSets.set(visibilityGetters = new Map());
// Relies on the name being exposed.
visibilityGetters.set(context.name, accessors.get);
return accessors;
};
visible.define = (klass, context, placeholder) => {
const visibilityGetters = visibilityPrivates.get(placeholder);
if (!visibilityGetters) return;
for (const [fieldName, get] of visibilityGetters) {
Object.defineProperty(klass.prototype, fieldName.slice(1), {
get,
});
}
return klass;
};
const {extended} = Placeholder();
@extended(visible.define)
class Foo {
@extended(visible) #bar = someSecretData;
} In this case I'm not sure how the |
@mhofman, I have a couple of questions:
|
The placeholder is just an object that stands in place of the class. What the decorators do with it is up to them. Since the placeholder object is shared between the invocation of all decorators (fields, methods, class, and if added, initializer) for that class, the decorators can either use it as a private Map key, or add properties directly to the object. Does that answer your question? |
Yes, thanks. I have the following thoughts about it.
However, I think I like this proposal. In some parts it is even better than mine. I like the concept of a unique object per class declaration that can be used as an unique token to bind decorators together. And it allows protecting the data even better. |
Right, and to make things easier, the order of execution of decorators should be clearly specified. However as the userland "polyfill" I shared above shows, the execution order is already observable, so this is not exposing anything new. IMO, the following order would work:
The above doesn't remove the need for decorators to be resilient to arbitrary order if they need to collaborate on the same type of data, e.g. the getter and the setter calls may come in any order.
Correct. This placeholder logic is in addition of the metadata, which is still valuable for attaching public data to the class. Also the decorators have access to the
Agreed. It was just easier to monkey patch above in my userland implementation. |
Sorry for being unclear. I meant the order of member decorators. If they are called in order of delcaration, it may cause execution race if these decorators are able to both write and read the metadata of others. E.g., let's imagine that we have the following decorators: const wrapper= Symbol('wrapper');
function foo(f, context) {
context.metadata[wrapper] = createSomeWrapperFrom(f);
return f;
}
function bar(f, context) {
const wrapper = context.metadata[wrapper];
return wrapper(f);
} Then, if we call them in the proper order, everything is ok: class Foo {
@foo fuzz() {}
@bar buzz() {}
} However, if we call them in the wrong order, everything fails: class Foo {
// Throws an error about `wrapper` being undefined
@bar fuzz() {}
@foo buzz() {}
} That's why current proposal completely remove the race origin making member decorators unable to read metadata of any member decorators. Only class decorators are able to read that data.
Isn't it too complicated then? I thought that metadata object is going to replace |
Data can currently be shared across decorators of the same element via metadata. Also the proposal has been updated since this issue was made, |
I have to admit I haven't kept up with this proposal lately, but given the feedback on metadata at the plenary, I'm wondering if an approach like a placeholder would allow to implement metadata in userland. |
I believe a few use-cases are currently hard to implement with the new proposal, but could be made more straightforward with a mechanism to share data across multiple decorators:
@visible
decorator that would create a getter for a private field (make it publicly read-only)@init: decorator
syntax and"init-method"
type, but doesn't cover accessors.To avoid unexpected mutations, the current proposal doesn't pass the decorator any reference to the class being decorated, which means decorators have no idea of the context in which they're run. To collaborate, multiple decorators can rely on the
metadata
(which is what #319 suggests), however the metadata lookup can only be performed if one has a reference to the class, limiting the use case to extending the original class. Furthermore, themetadata
is public, preventing any private sharing.If calls to decorators included a "placeholder" / shared context object, it would allow them to collaborate. This placeholder wouldn't hold any reference to the class, but would be a unique object standing on its behalf, shared across all decorator calls for that class. A decorator could add a property to that placeholder object that another decorator can use. Decorators worried about exposing their internals to unrelated decorators can use a simple
WeakMap
to associate their data with the placeholder.The above use cases would be solved as follow:
@visible
decorator would attach the private field "get" method to the placeholder object. A@visible.define
decorator on the class would extend the class and define a getter for all fields saved on the shared placeholder object.@bound
or@on
would respectively save the method name, or the event name and method on the placeholder object. a@bound.init
or@on.init
would be used on the class definition to extend the class to perform the needed initialization in the constructor (similar to the mix-in pattern in the Readme). To avoid unnecessary class extensions, this pattern could be standardized with constructor decorators (with a new "initialize" type?) that would execute a returned function during the instance construction (before or aftersuper()
TBD)The text was updated successfully, but these errors were encountered: