Skip to content
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

associated-opaque-object metadata #5

Open
bakkot opened this issue Mar 29, 2022 · 37 comments
Open

associated-opaque-object metadata #5

bakkot opened this issue Mar 29, 2022 · 37 comments

Comments

@bakkot
Copy link

bakkot commented Mar 29, 2022

I am still not convinced metadata is necessary at all.

However, in tc39-delegates we did discuss an alternative design for metadata, simpler than the design currently in this proposal and different from tc39/proposal-decorators#451, which I want to sketch here:

Every class would have an associated object, automatically placed on the class's Symbol.metadata field, which would be a frozen ordinary object with one property, parent, which is either null (for base classes) or the value of the superclass's Symbol.metadata (for derived classes).

This object would be provided on the context passed to every decorator as (e.g.) context.metadata. The same value would be used for class, static, and instance decorators.


That's it. Decorators could use this to coordinate by using the associated object as a key in a WeakMap. The benefits of this design are

For example, the DI example from the readme would look like

const injections = new WeakMap;

function inject(injectionKey) {
  return (value, context) => {
    const { set } = context.access;

    if (!injections.has(context.metadata)) {
      injections.set(context.metadata, []);
    }
    injections.get(context.metadata).push({ injectionKey, set })
  }
}

class Container {
  registry = new Map();

  register(injectionKey, value) {
    this.registry.set(injectionKey, value);
  }

  lookup(injectionKey) {
    this.register.get(injectionKey);
  }

  create(Class) {
    let instance = new Class();

    let pointer = instance.constructor[Symbol.metadata];
    while (pointer != null) {
      for (let { injectionKey, set } of injections.get(pointer) ?? []) {
        set.call(instance, this.lookup(injectionKey));
      }
      pointer = pointer.parent;
    }

    return instance;
  }
}

class Store {}

class C {
  @inject('store') #store;

  get store() {
    return this.#store;
  }
}

let container = new Container();
let store = new Store();

container.register('store', store);

let c = container.create(C);

c.store === store; // true

(This is similar to tc39/proposal-decorators#328, though not quite identical.)

@ljharb
Copy link
Member

ljharb commented Mar 29, 2022

What would injectionKey be?

I'd hope it's a Symbol, but obv it's potentially an issue without the ability to reference them weakly.

@bakkot
Copy link
Author

bakkot commented Mar 29, 2022

That's not something in this API; it's a closed-over parameter in my example code.

@ljharb
Copy link
Member

ljharb commented Mar 29, 2022

right, but it still has to be something - what is it? i guess in this example it's "any object"?

@bakkot
Copy link
Author

bakkot commented Mar 29, 2022

I think there's some confusion here. injectionKey in my sample code is not part of the API. It is purely a thing in the sample code.

When I write @inject('store'), that calls the function inject(injectionKey) { /* ... */ } defined above, with 'store' as the sole argument. So within the body of inject, injectionKey has the value 'store'. Because that's how functions in JavaScript work.

@justinfagnani
Copy link

This is great! I believe it would solve all of our metadata needs. Any cross-library needs should be met by using a common userland metadata library.

@mhofman
Copy link
Member

mhofman commented Mar 29, 2022

Having proposed a similar idea idea in tc39/proposal-decorators#328, I like the ergonomics of this approach, in particular that non public coordination is the default.

I am however wondering how we could tweak this so that method specific method metadata may exist in the future. For example a validation library could do:

import { validate, string, number } from 'validation-lib';

class Foo {
  @validate bar(@string name, @number size) {
    // name is guaranteed to be typeof 'string'
    // size is guaranteed to be typeof 'number'
  }
}

One way might be to simply be more explicit in the name, and have separate context.classMetadata and context.functionMetadata

@senocular
Copy link

Is it just me or does it seem odd for classes to be given a Symbol.metadata field that neither contains nor can be given metadata.

@acutmore
Copy link

Is it just me or does it seem odd for classes to be given a Symbol.metadata field that neither contains nor can be given metadata.

Maybe the symbol should have a different name if using this approach? As you say, the value is not metadata but a unique identifier that also captures the hierarchy. Symbol.context, Symbol.decoratorContext maybe?

@bakkot
Copy link
Author

bakkot commented Mar 29, 2022

Yeah it would probably want a different name; metadataKey or something would be more obvious.

@domenic
Copy link
Member

domenic commented Mar 29, 2022

Every class would have an associated object, automatically placed on the class's Symbol.metadata field, which would be a frozen ordinary object with one property, parent, which is either null (for base classes) or the value of the superclass's Symbol.metadata (for derived classes).

Does this/should this include built-in ECMAScript classes like Map, Promise, Array, and Number? What about web platform classes or other host classes?

@bakkot
Copy link
Author

bakkot commented Mar 29, 2022

By "class" I intended syntactic class specifically, which are the only thing decorators be applied to.

But now that you mention it I guess I could hypothetically see value in this available for built-in classes as well; I'd have to think about it. I'd lean towards not doing anything there unless we have a specific use case in mind.

@domenic
Copy link
Member

domenic commented Mar 29, 2022

Yeah, the motivating question is what the following returns:

(class extends Map { })[Symbol.metadata].parent

If it's null that's probably fine, it's just another minor asymmetry between built-ins and user classes. (And not a fundamental one; one could mimic a built-in by doing delete MyClass[Symbol.metadata], IIUC.)

@bakkot
Copy link
Author

bakkot commented Mar 29, 2022

Yeah, I'd think that would be null (or undefined, I guess).

From the perspective of a decorator author I think that would not cause any problems; the thing you would use the parent key for is to look up metadata from decorators, and there's no such data for the built-in classes. That is, there's no important difference to a decorator author whether someone has done class extends Map {} or class { #wrapped = new Map }, with regards to the parent.

(And not a fundamental one; one could mimic a built-in by doing delete MyClass[Symbol.metadata], IIUC.)

Yes, or by using a syntactic function () {}, for that matter.

@rbuckton
Copy link

An opaque object generally assumes that metadata should only be produced/consumed by the metadata library. However, that isn't the only use case for metadata. A compiler, might want to be able to attach design-time metadata to a class that can be publicly consumed (as is the case today with --emitDecoratorMetadata in TS). A compiler may be constrained to emit code that can be run regardless as to whether the file is a Script or a Module, and therefore may not be able to depend on the presence of a userland library with which to make such metadata available.

With a mutable metadata object TypeScript (or any other compile-to-JS compiler) could, for example, emit design-time metadata inline:

// input
class C {
  @RuntimeChecks // some userland decorator
  add(x: number, y: number) { return x + y; }
}

// output
class C {
  @RuntimeChecks
  @((_, context) => {
    const members = context.metadata["ts:TypeInfo"] ??= {};
    members["add"] = {
      "ParameterTypes": () => [Number, Number],
      "ReturnType": () => Number,
    };
  })
  add(x, y) { return x + y; }
}

If the metadata isn't easily writable, such a capability would require shimming a global to allow it to be used in Script. This is unfortunately the approach we use today with libraries like reflect-metadata, and its something we had hoped to move away from if there was a built-in mechanism for defining metadata.

And this isn't limited to providing runtime addressable type information. There are other use cases, such as:

  • Recording marshaling information for something like a compile-to-WASM compiler with a JS/WASM interop layer
  • Attaching extra debug information to be used by debuggers to control watch/locals previews

@rbuckton
Copy link

One way might be to simply be more explicit in the name, and have separate context.classMetadata and context.functionMetadata

I just filed tc39/proposal-decorators#466 today, which proposes a similar bifurcation, i.e., have context.metadata for the function and context.class.metadata for the class (as well as context.class.name for the class name).

@bakkot
Copy link
Author

bakkot commented Apr 12, 2022

A compiler which emits JS can do whatever it wants; it doesn't need to depend on syntax in the language to provide functionality. For example, your snippet can be achieved with this proposal as it stands today as

// input
class C {
  @RuntimeChecks // some userland decorator
  add(x: number, y: number) { return x + y; }
}

// output
const tsMetadata = {
  add: {
    "ParameterTypes": () => [Number, Number],
    "ReturnType": () => Number,
  },
};
const runWithMetadata = f => (v, c) => f(v, { tsMetadata, ...c });
class C {
  @runWithMetadata(RuntimeChecks)
  add(x, y) { return x + y; }
}

or similar. There is no particular reason to rely on decorators to do this transformation when you can just emit code which does it directly. As such, I don't think compilers make for a very compelling use case.

For other use cases, which don't have the constraint of not using libraries at runtime, I think it's more reasonable to provide access to metadata via the library itself, rather than by trying to put data on the class instance or the shared context object. I really don't think it's a good idea to introduce a new shared string-based global namespace, which is what a mutable metadata object would amount to.

@rbuckton
Copy link

rbuckton commented Apr 13, 2022

I'm not sure where you're getting a "shared string-based global namespace" from my example. Such metadata would live on the class itself, not globally. My intent is to allow a decorator to attach information to a class that can be read via reflection/introspection against the class itself. One such case is a userland decorator consuming this information. Other cases include passing a class constructor to another API which might need to perform similar introspection, as is often the case with Dependency Injection and with ORM.

Your counter-example does not solve for those cases. It also doesn't solve for the original case, for several reasons:

  • The metadata would only be available to the currently executing decorator, rather than all decorators of the class.
  • It would require wrapping every decorator with a runWithMetadata call, which is inefficient.

There is no particular reason to rely on decorators to do this transformation when you can just emit code which does it directly.

Given the execution steps for class definition evaluation, the only way for this information to be available to a decorator and meet these needs is if it is generated by another decorator that is applied prior to it.

[...] I don't think compilers make for a very compelling use case.

There are a number of languages with overlapping prior-art (C# Attributes, Java Annotations, Python Decorators, etc.). In many cases the compilers for these languages can and do inject such attributes and annotations at compile time. TypeScript's own --emitDecoratorMetadata is an example of how this could be used in the future, and there are a number of projects that leverage this capability. I would certainly not write off compilers as a compelling use case.

@bakkot
Copy link
Author

bakkot commented Apr 13, 2022

I'm not sure where you're getting a "shared string-based global namespace" from my example.

Sorry, let me clarify: if all decorators from all libraries are contending over the string-named properties of a shared metadata object, then "the names of properties on metadata objects" is a shared namespace. For example, if Alice and Bob both write decorators which use the "props" field of a metadata object, and they use it in a different way, then these decorators can't be used on the same class. They are contending over the same namespace. I do not think we should introduce any new such namespaces.

Other cases include passing a class constructor to another API which might need to perform similar introspection, as is often the case with Dependency Injection and with ORM.

A compiler which wants to attach data to the class constructor itself, to be available after the class is constructed, can add fields to the class. A compiler which wants to make additional data available to decorators can transform the decorators. A library can expose data via the library itself, both during and after the decorators phase, since it does not have the constraint that the data needs to be available without importing a library.

  • The metadata would only be available to the currently executing decorator, rather than all decorators of the class.
  • It would require wrapping every decorator with a runWithMetadata call, which is inefficient.

If you wrap every decorator on the class, then it's available to every decorator, surely? And I really don't think the inefficiency, performance-wise, would actually be that significant.

Given the execution steps for class definition evaluation, the only way for this information to be available to a decorator and meet these needs is if it is generated by another decorator that is applied prior to it.

Then perhaps I don't understand the ask. My example includes only a single decorator, yet that decorator has additional information available to it. If you have a different example in mind where that doesn't work, can you give it?

In many cases the compilers for these languages can and do inject such attributes and annotations at compile time.

I don't know what this means. Decorators are a syntactic feature; to say they are injected "at compile time" is only coherent if the compiler is generating source code in the language in question, which is how TS works but typically not Java or Python.

@bakkot
Copy link
Author

bakkot commented Apr 22, 2022

This is now being discussed at https://github.com/tc39/proposal-decorator-metadata.

@ljharb ljharb transferred this issue from tc39/proposal-decorators Apr 22, 2022
@jalligator
Copy link

jalligator commented Jun 7, 2022

I've spent time thinking about this as a lot of my decorators rely a lot on identifying the class. Not that I'm proposing a solution, but I do think it's worth thinking about this being nothing more than a closure on a symbol. For instance:
`
import { SpecialDecorators } from '/special-decorators';

let decorators = new SpecialDecorators(Symbol());
// really, you can just have the Symbol spun up in the constructor. This is just to highlight that
// the closure depends on a unique identifier

@decorators.forClass('what-a-class')
export default class Clazzy {
@decorators.forField field;
}
`

All the decorators then have access to the bound "symbol." Then you could easily link to a map or whatever. In this case, I used a string. It doesn't matter. The idea is that it's totally lexically possible to accomplish the goal of this proposal rather easily. It's really just a matter of syntax to get rid of the instantiation of a decorator around a symbol.

`class MyDecorators {

greeting = "Hey you, ";

constructor(greeting){
this.greeting = greeting;
}

forClass = function(...args){
console.log(this.greeting + 'from class');
console.log(args);
}.bind(this);

forMethod = function(...args){
console.log(this.greeting + 'from method');
console.log(args);
}.bind(this);
}

let decs = new MyDecorators("Hello, ");`

The output is as you'd expect it.

It works in legacy anyhow. I honestly don't know if the proposed changes to transpilation would make this line of thinking invalid.

@mhofman
Copy link
Member

mhofman commented Jun 7, 2022

The opaque object approach is meant to enable exactly these kind of use cases without relying on awkward decorator instantiation / binding, and without requiring a compilation step (which really is the point of decorators in the first place, as anything they do can technically be compiled away)

Is there any concern with the opaque object approach described in this issue. Would it work for your use case?

@jalligator
Copy link

Not at all. I think it’s perfect, actually. But I was having a bit of a hard time getting my head around it until I thought of it in the way I described. Just sharing my musings (and looking for some validation) in case it helps someone else.

@rbuckton
Copy link

rbuckton commented Sep 8, 2022

I apologize for this reply being a bit long-winded, but there are a lot of different pieces to consider related to how TypeScript would like to be able to use decorator metadata to emit Run-time Type Information (RTTI). The goal would be to have something that has moderately clean emit, is relatively easy to emulate in native JS for cross-compatibility, and isn't terribly burdensome for decorator maintainers.

Before I get into the various options, please consider the following TypeScript definition:

class Point {
    @dec
    x: number;

    @dec
    y: number;

    @dec
    offset(p: Point): Point { ... }

    ...
}

With a mutable context.metadata object, the RTTI emit for Point might look like the following:

class Point {
    @dec // type info is immediately available to `@dec` via `context.metadata`
    @((_, context) => {
        context.metadata["design:typeinfo"] ??= { instance: {} };
        context.metadata["design:typeinfo"].instance.x = { type: () => Number };
    })
    x;

    @dec // type info is immediately available to `@dec` via `context.metadata`
    @((_, context) => {
        context.metadata["design:typeinfo"] ??= { instance: {} };
        context.metadata["design:typeinfo"].instance.y = { type: () => Number };
    })
    y;


    @dec // type info is immediately available to `@dec` via `context.metadata`
    @((_, context) => {
        context.metadata["design:typeinfo"] ??= { instance: {} };
        context.metadata["design:typeinfo"].instance.offset = {
            type: () => Function,
            parameters: [{ type: () => Point }],
            return: { type: () => Point },
        };
    })
    offset(p) { ... }

    ...
}

Ideally, the type information we emit should be immediately available to decorators of the object, providing access to RTTI before chosing to replace a decoration target or add an initializer. It also means that the decorator can access RTTI information from the declaration site as opposed to needing to depend on a this lookup (and prototype walk) during execution. This also makes it easy to access this information as a maintainer.

Consider a checked decorator that wraps a method with one that does runtime type checking (RTTC):

function checked(target, context) {
    if (context.kind === "method" && !context.private) {
        // happens *once* during declaration
        const typeinfo = context.metadata["design:typeinfo"];
        const parmeters = typeinfo?.[context.static ? "static" : "instance"]?.[context.name]?.parameters;
        // *conditionally* wrap the method
        if (parameters) {
            return function (...args) {
                // ... check each arg against its expected parameter type
                return target.call(this, ...args);
            };
        }
    }
}

The above decorator can access the RTTI object outside of the replacement function, based on the lexical declaration of the method on the containing class. It turns out this is a very valuable capability, as getting to this information without immediate availability is significantly more complex.

If context.metadata ends up being immutable, its unlikely that TypeScript will be able to use it at all to emit RTTI. If we still want RTTI available immediately, we would have to wrap each decorator call as proposed by @bakkot:

const _Point_typeinfo = { instance: {} };
class Point {
    static ["design:typeinfo"] = _Point_typeinfo;

    // type info is immediately available to `@dec` via `context["design:typeinfo"]`
    // wrap decorator, attaching a custom property with a name unlikely to conflict with any future proposal
    @((_dec => (target, context) => {
        context["design:typeinfo"] = _Point_typeinfo; 
        return _dec(target, context);
    })(dec))
    @(() => {
        _Point_typeinfo.instance.x = { type: () => Number };
    })
    x;

    @((_dec => (target, context) => {
        context["design:typeinfo"] = _Point_typeinfo;
        return _dec(target, context);
    })(dec))
    @(() => {
        _Point_typeinfo.instance.y = { type: () => Number };
    })
    y;

    @((_dec => (target, context) => {
        context["design:typeinfo"] = _Point_typeinfo;
        return _dec(target, context);
    })(dec))
    @(() => {
        _Point_typeinfo.instance.offset = {
            type: () => Function,
            parameters: [{ type: () => Point }],
            return: { type: () => Point }
        };
    })
    offset(p) { ... }

    ...
}

This has a very signifcant drawback: Decorator maintainers shouldn't necessarily need to depend on TypeScript-specific emit to function correctly. If we depend on a property injected into the context, a JS-only project would have to do the same thing manually to leverage checked, above.

But, what would it look like if this metadata wasn't available immediately? The emit for RTTI would certainly be much simpler:

class Point {
    static ["design:typeinfo"] = {
        instance: {
            x: { type: () => Number },
            y: { type: () => Number },
            offset: {
                type: () => Function,
                parameters: [{ type: () => Point }],
                return: { type: () => Point },
            }
        }
    };

    @dec // type info is not immediately available to `@dec`
    x;

    @dec // type info is not immediately available to `@dec`
    y;

    @dec // type info is not immediately available to `@dec`
    offset(p) { ... }
}

However, consuming this type information is significantly more complex:

function checked(target, context) {
    if (context.kind === "method" && !context.private) {
        let foundClass, parameters;
        // *always* wrap the method
        return function (...args) {
            if (!foundClass) {
                let current = this;
                while (current) {
                    const constructor = context.static ? current : current.constructor;
                    if (constructor[Symbol.metadata] === context.metadata) {
                        const typeinfo = Object.getOwnPropertyDescriptor(constructor, "design:typeinfo")?.value;
                        parmeters = typeinfo?.[context.static ? "static" : "instance"]?.[context.name]?.parameters;
                        foundClass = true;
                        break;
                    }
                    current = Object.getPrototypeOf(current);
                }
            }

            if (parameters) {
                // ... check each arg against its expected parameter type
            }
            return target.call(this, ...args);
        };
    }
}

The complexity above stems from the fact that we need to do a prototype walk to ignore subtypes, an own-property check to ignore supertypes, and caching so that we don't re-evaluate this work on every method call. The prototype walk is important because a subclass might override the method with one that accepts a looser type for a parameter, but that type should only apply to the overridden method:

class Super {
    @checked
    accept(x: string) { ... }
}

class Sub extends Super {
    @checked
    accept(x: string | number) { super.accept(`${x}`); }
}

Without the prototype walk, the @checked on Super.prototype.accept might mistakenly allow number values.

And this doesn't even work reliably due to .call({}) and the constructor return override trick!

Given this complexity, we might instead turn to addInitializer to try to get the RTTI object at something close to declaration time. But if addInitializer does not allow you to specify timing, this only simplifies the case for static members since addInitializer for non-static members runs per-instance:

function checked(target, context) {
    if (context.kind === "method" && !context.private) {
        let foundClass, parameters;
        context.addInitializer(function () {
            if (context.static) {
                // ok, this one's moderately easy but we still need to make sure we're looking at an *own*
                // property and not an inherited one.
                const typeinfo = Object.getOwnPropertyDescriptor(this, "design:typeinfo")?.value;
                parmeters = typeinfo?.[context.static ? "static" : "instance"]?.[context.name]?.parameters;
            }
            else {
                // uh oh, we *still* need to do a prototype walk because initializers for non-static members are run
                // per instance, meaning `this` could be a subclass...
                if (!foundClass) {
                    // need to walk prototype chain to correctly handle inheritence
                    let current = this;
                    while (current) {
                        const constructor = context.static ? current : current.constructor;
                        if (constructor[Symbol.metadata] === context.metadata) {
                            const typeinfo = Object.getOwnPropertyDescriptor(constructor, "design:typeinfo")?.value;
                            parmeters = typeinfo?.[context.static ? "static" : "instance"]?.[context.name]?.parameters;
                            foundClass = true;
                            break;
                        }
                        current = Object.getPrototypeOf(current);
                    }
                }
            }
        });
        // *always* wrap the method
        return function (...args) {
            if (parameters) {
                // ... check each arg against its expected parameter type
            }
            return target.call(this, ...args);
        };
    }
}

So addInitializer doesn't help us unless we're able to specify when the initializer runs, as anything else is too complex and easy to do incorrectly.

In the end, we're left with one of the following options:

  1. Push for a mutable context.metadata object.
    • relatively simple emit
    • easy to emulate RTTI in native JS
    • easy to access reliably
  2. Wrap every decorator call to patch a custom property into the context object.
    • only really works well for TS users since its harder to emulate RTTI in native JS due to injected context["design:typeinfo"] property
  3. Push for configurable addInitializer instead of context.metadata.
    • very simple emit
    • moderately easy to access reliably
  4. Don't make RTTI available immediately to decorators.
    • very simple emit
    • excessively complex to access reliably

If we want something that has moderately clean emit, is relatively easy to emulate in native JS for cross-compatibility, and isn't terribly burdensome for decorator maintainers, then the only options that match that are (1) or (3).

@rbuckton
Copy link

rbuckton commented Sep 8, 2022

Regarding native JS emulation of RTTI, with a mutable context.metadata this is fairly easy to achieve in userland:

function getOrCreateMemberInfo(context) {
    if (context.private) throw new TypeError("Not supported on private members");
    const typeinfo = context.metadata["design:typeinfo"] ??= {};
    if (context.kind === "class") {
        return typeinfo.class ??= {};
    } else {
        const placement = context.static ? "static" : "instance";
        const members = typeinfo[placement] ??= {};
        return members[context.name] ??= {};
    }
}

function Type(typeCallback) {
    return (_, context) => {
        getOrCreateMemberInfo(context).type = typeCallback;
    };
}

function ParameterTypes(typeCallbacks) {
    return (_, context) => {
        getOrCreateMemberInfo(context).parameters = typeCallbacks;
    };
}

function ReturnType(typeCallback) {
    return (_, context) => {
        getOrCreateMemberInfo(context).return = typeCallback;
    };
}

class Point {
    @dec
    @Type(() => Number)
    x;

    @dec
    @Type(() => Number)
    y;

    @dec
    @Type(() => Function)
    @ParameterTypes([() => Point])
    @ReturnType(() => Point)
    offset(p) { ... }
    ...
}

@bakkot
Copy link
Author

bakkot commented Sep 8, 2022

Thanks for the writeup, @rbuckton. I want to focus on the "immutable metadata key, TS wraps decorators" case, and in particular to focus on what "native JS emulation of RTTI" looks like in that case.

With either mutable or immutable metadata keys, using a decorator which expects TS to inject metadata without actually using TS requires the class author to provide the metadata manually in some fashion. Above you illustrate how it can be done in a "mutable metadata key" world by using a @Type decorator to mutate the metadata key. With an immutable metadata key, that approach doesn't work, but you can still do it like this:

let WithType = type => dec => (v, ctx) => dec(v, { 'design:typeinfo': type, ...ctx });

class Point {
  @WithType(() => Number)(dec)
  x;
}

This is for the worst case, where the @dec decorator has been written assuming it's definitely being used with TypeScript as a transpiler but the user still wants to use it in a plain JS file. I don't think that's the best design, but I did want to illustrate that it's still doable - it just requires the person manually writing JS to use @WithType(t)(dec) instead of @Type(t) @dec.


But if the @dec decorator is willing to cooperate a bit, you can use a library to achieve exactly the same user experience (for the class author) as before:

// type-lib
let types = new WeakMap;

export function getType(ctx) {
  // use TS-injected metadata if available
  if (Object.hasOwn(ctx, 'design:typeinfo')) return ctx['design:typeinfo'];

  // otherwise fall back to the WeakMap, assume class author has used a `@Type` decorator themselves
  return types.get(ctx.metadataKey);
}

export let Type = typeCallback => (_, ctx) => void types.set(ctx.metadataKey, typeCallback);
// decorator author
import { getType } from 'type-lib';

export function dec(v, ctx) {
  let type = getType(ctx); // use this helper instead of accessing the `design:typeinfo ` property directly
  // ...
}
// decorator usage, raw JS
// exactly the same as in the "mutable metadata key" case
import { Type } from 'type-lib';
class Point {
  @Type(() => Number)
  @dec
  x;
}

That seems like it's pretty much as good as the "mutable metadata key" example you give above. The only downside is that there has to be coordination around type-lib - but there has to be a Type decorator in some library either way, and it doesn't seem so bad for the decorator author as well as the decorator consumer to need to import that library.

@justinfagnani
Copy link

I actually think that the @type() decorator may be a good idea even for code compiled by TypeScript. It would be a per-field opt-in to RTTI, rather than a compilation-unit flag.

@rbuckton
Copy link

rbuckton commented Sep 8, 2022

That still doesn't put us in much better of a situation than we are with reflect-metadata and stage 1 decorators. You need to import a 3rd party package to use it in a module, or patch a global to use it in a Script. Avoiding both of these are among reasons I pushed for Metadata in the decorators proposal in the first place.

@justinfagnani
Copy link

Metadata is needed in the decorators proposal because otherwise there's no way for decorators to add any kind of metadata, even visible only to that particular decorator.

This is a slightly different problem where you want metadata provided without decorators, yes?

@rbuckton
Copy link

rbuckton commented Sep 9, 2022

I actually think that the @type() decorator may be a good idea even for code compiled by TypeScript. It would be a per-field opt-in to RTTI, rather than a compilation-unit flag.

It is already essentially a per-field opt-in because it only applies to members with decorators when the --emitDecoratorMetadata flag is set. Requiring users to explicitly write @type decorators to provide existing type information is redundant and could easily result in mismatched values.


This is a slightly different problem where you want metadata provided without decorators, yes?

No, I still want metadata provided using decorators.


With an immutable metadata key, that approach doesn't work, but you can still do it like this: [...]

I am not thrilled with the idea of having to patch the context object, as that approach feels very fragile. It also means that TS wouldn't be able to use the same concept of "metadata" as anyone else.

I think if you are writing decorators in your own application (a frequent occurrence in many repos), being able to simply attach metadata via context.metadata.someKey is perfectly acceptable. It should be up to the authors of decorator libraries to use context.metadata in a way that doesn't result in collisions (i.e., using a namespaced or uri-qualified string as a key, or a symbol, etc.). If you are concerned about your metadata leaking, that's when you should use a WeakMap and use context.metadata as the key. Forcing everyone to use a WeakMap feels like overkill.

@bakkot
Copy link
Author

bakkot commented Sep 12, 2022

I am not thrilled with the idea of having to patch the context object, as that approach feels very fragile.

In what way? Do you think ES is likely to start adding a design:typeinfo key, or is there some other sort of fragility you're concerned about?

I think if you are writing decorators in your own application (a frequent occurrence in many repos), being able to simply attach metadata via context.metadata.someKey is perfectly acceptable.

I agree that if you are coordinating only with yourself, using a shared namespace is not problematic. But I don't think we should optimize for this case, particularly for decorators.

It should be up to the authors of decorator libraries to use context.metadata in a way that doesn't result in collisions [...] If you are concerned about your metadata leaking, that's when you should use a WeakMap

We can design this feature such that collisions are impossible by nature, and so that library-specific data is only exposed intentionally, or we can rely on authors of decorators to figure out some mechanism to handle collisions themselves, and trust that they will remember that the API is public by default and remember to restrict it when that is not intentional.

The first thing seems obviously better to me. To pick the second I would want there to be some commensurate benefit to that approach. And I don't yet see that benefit. TypeScript will need to do slightly less work with the second approach, but it remains feasible with the first, and TypeScript is in an extremely unusual position; the only other upside is that decorator authors don't need to understand WeakMaps, but that does not seem so significant to me.

Do you think there is some other benefit which makes the cost worthwhile, or do you disagree with my weighting of the costs and benefits I've listed here?

@PodaruDragos
Copy link

Hello, I have a question ( I don't know if this is the correct place to ask this ) but here goes
Since babel and typescript are going to implement stage 3 decorators, Is there any timeline for when this proposal which I think is crucial for typescript especially would be presented to move to stage 3 also ?

@pzuraq
Copy link
Collaborator

pzuraq commented Nov 29, 2022

@PodaruDragos I think it would be better to open a new issue since the question is unrelated to this thread, but a quick status update:

  • I'm the current champion for this proposal, but due to time constraints with my job I haven't had a ton of time to work on it.
  • I'm planning on finding some time around the holidays this year and early next quarter to solidify the draft, update the spec text, and get it ready to present to committee.
  • If all goes well, I hope to present an update by the January meeting. This would not be the meeting when we would move to stage 3, since there will be some major changes to the spec, it would just be an update.

Presenting for stage 3 would have to happen after that meeting sometime, so the earliest it might move forward is sometime in early-mid 2023, but that depends on me finding time to do the above work and present.

FWIW, my current plan is to update this proposal based on the design proposed by @rbuckton in this thread, where context.metadata will be a mutable object whose prototype is the parent class's metadata object. I think the tradeoffs presented in this thread make sense on both sides, but I find that the advantages of the mutable metadata object proposal are very compelling:

  1. It's simpler and easier to understand overall
  2. It allows for emergent standards and shared metadata to evolve over time
  3. The inheritance model feels much closer to classes overall, using prototypes here feels right

I understand the concerns with introducing a new namespace, but my feeling is that the advantages outweigh the potential issues. We'll be debating all this in committee and in the decorators meetings, of course, I'm sure @bakkot would like to discuss this in more detail.

@rbuckton
Copy link

rbuckton commented Mar 7, 2023

I've said some of this in #12 (comment), but I'm posting it here as well.

I am not thrilled with the idea of having to patch the context object, as that approach feels very fragile.

In what way? Do you think ES is likely to start adding a design:typeinfo key, or is there some other sort of fragility you're concerned about?

Patching the context object is a no-go, as we're trying to avoid introducing new TypeScript-specific runtime behavior that conflicts with native behavior. That is something that has been strongly contraindicated by many in the JS/TS community as well as other members of TC39. Such patching would also result in decorators that only work when used in TypeScript, resulting in further bifurcation of the ecosystem. It does not align with our goals, nor with the goals of proposals like Type Annotations.

I think if you are writing decorators in your own application (a frequent occurrence in many repos), being able to simply attach metadata via context.metadata.someKey is perfectly acceptable.

I agree that if you are coordinating only with yourself, using a shared namespace is not problematic. But I don't think we should optimize for this case, particularly for decorators.

I don't consider this "optimizing" for a specific use case. I believe this is a perfectly reasonable, and acceptable approach. A frozen object is an artificial limitation that only affects those that might wish to use, or can only use, a mechanism that relies on a mutable object. If you want isolation, you can still use a WeakMap regardless as to whether the object is frozen or not. You are welcome to discourage its use as a shared namespace within your own projects, but there is no need to enforce such a constraint on the entire ecosystem.

It should be up to the authors of decorator libraries to use context.metadata in a way that doesn't result in collisions [...] If you are concerned about your metadata leaking, that's when you should use a WeakMap

We can design this feature such that collisions are impossible by nature, and so that library-specific data is only exposed intentionally, or we can rely on authors of decorators to figure out some mechanism to handle collisions themselves, and trust that they will remember that the API is public by default and remember to restrict it when that is not intentional.

The first thing seems obviously better to me. To pick the second I would want there to be some commensurate benefit to that approach. And I don't yet see that benefit. TypeScript will need to do slightly less work with the second approach, but it remains feasible with the first, and TypeScript is in an extremely unusual position; the only other upside is that decorator authors don't need to understand WeakMaps, but that does not seem so significant to me.

A "shared namespace" is extremely useful for sharing information between disparate libraries without introducing unnecessary dependencies. A mutable metadata object is extremely useful to attach metadata in a Script context where you may not be able to readily depend on an ImportCall.

There is an entire ecosystem of thousands of projects that are written in TypeScript today that leverage reflect-metadata, which is essentially a "shared namespace". One of the original (and current) goals of the metadata proposal was to facilitate migration from TypeScript's legacy decorators and reflect-metadata to native decorators and native metadata. That migration is not feasible with a frozen metadata object, at least, not in any way that has been suggested so far in this thread, and not without TypeScript introducing some new non-compliant runtime behavior.

Do you think there is some other benefit which makes the cost worthwhile, or do you disagree with my weighting of the costs and benefits I've listed here?

Yes, to both, as I disagree with your weighting as you are not taking into account the existing ecosystem and the feasibility of migration.

@justinfagnani
Copy link

At this point, I think a mutable metadata object is the best path forward.

Yes, it's possible to use in an unsafe way, as is much of JavaScript, but it's important to consider that it's also easy to use in a safe way via weak maps or symbol-keys. As a maintainer of a library that heavily uses decorators and metadata I will choose the safest, most encapsulated, pattern. Others can choose what's best for their requirements.

I've very sympathetic to TypeScript's needs here around a shared namespace. When that's what you need there's simply no real substitute. Even if we restricted the metadata API to either be immutable or require Symbol keys, TypeScript would still have to use a shared namespace:

  • For a Symbol-based API, you would still have a shared namespace via Symbol.for()
  • For an immutable object, you would need to coordinate with a global, like window._TypeScriptMetadata ??= new WeakMap() which is still a shared namespace.

So there's no fundamental difference, and being too strict about metadata doesn't actually make the API safe.

@pzuraq
Copy link
Collaborator

pzuraq commented Mar 7, 2023

While I agree with the point that with Symbol.for() and Object.getOwnPropertySymbols(), there is no such thing as a truly private symbol, I do believe that the Symbol version is different from the plain mutable object version for two main reasons:

  1. It requires the author to step back and think about whether or not they want their metadata to be public. The author must either type Symbol() and create their unique symbol, or Symbol.for() and think about what shared key they want to use. I do believe that many authors who would jump straight to using a string because it is the most natural pattern would step back and think about the difference between the two if they were forced to use symbols, and many would opt to use Symbol() over Symbol.for().
  2. In the case of authors using Symbol(), the unique symbol can still be accessed and overridden using Object.getOwnPropertySymbols(), and as noted previously Hyrum's law implies it probably will be in some cases. That said, it goes a long way to discourage that pattern, and also gives decorator authors an easy out when explaining away breaking changes. In the same way that a private property in TypeScript was technically public pre-private-fields, but it was a clear signal that it should not be used, a private symbol would encourage more decorator users and authors to avoid trying to work against the current, so to speak.

I think that these two factors remove the "public by default" problem. The reason a mutable object would be "public by default" and not "private by default with public as an option" is because it's simpler and easier to use string keys on an object than to bother with WeakMaps. If we raise the bar for using public metadata, all of a sudden it's no longer an easier choice. You can choose very public, somewhat public, and completely private, and all three are pretty much the same in terms of implementation complexity and reasoning. So, we would likely see a healthier ecosystem that chooses the right option for their use case, and not just the easiest option to implement.

That said, I also still favor the mutable object because I do think that most decorator authors will consider this difference and be intentional about their choices, and I think that it is a more intuitive model overall.

@bakkot
Copy link
Author

bakkot commented Mar 7, 2023

Patching the context object is a no-go, as we're trying to avoid introducing new TypeScript-specific runtime behavior that conflicts with native behavior. That is something that has been strongly contraindicated by many in the JS/TS community as well as other members of TC39.

Wait, I'm confused. This entire discussion has been about TS adding TS-specific runtime behavior - injecting metadata derived from the types into the runtime semantics. I understand the goal of avoiding TS-specific runtime behavior, but that goal has necessarily already been given up here, regardless of the approach taken.

"When using TS-with-metadata there is a new global (or new field on the context object) which doesn't come from any JS code" is exactly the same level of "new runtime behavior" as "when using TS-with-metadata there is a new field on the Symbol.metadata object which doesn't come from any JS code".

Such patching would also result in decorators that only work when used in TypeScript, resulting in further bifurcation of the ecosystem.

No, you can still make such a decorator work when used in raw JS, as I showed above.

I don't consider this "optimizing" for a specific use case. I believe this is a perfectly reasonable, and acceptable approach.

It's a reasonable approach in exactly the same way that global variables are a reasonable approach. If you're writing functions for your own application, sharing them via global variables is completely fine. The problems only arise when you start combining multiple pieces of code with different authors who aren't coordinating. But since "combining multiple pieces of code with different authors who aren't coordinating" is one of the fundamental things a programming language ought to enable, introducing a feature whose natural use will prevent that is bad.

This isn't something which can be fixed by a lint rule, because it's a coordination problem. Coordination problems are precisely those which no one notices when building their own component - they only become a problem when other people start trying to combine that component with others. Avoiding coordination problems requires the language to be designed in such a way that they do not arise.

That migration is not feasible with a frozen metadata object, at least, not in any way that has been suggested so far in this thread, and not without TypeScript introducing some new non-compliant runtime behavior.

I'm still not understanding why it's not feasible - I've sketched several solutions which seem to me to work fine. Perhaps it's a difference of opinion over what "new non-compliant runtime behavior" means? As far as I can tell adding a new field to the metadata object is exactly the same level of non-compliant as adding a new field to the context object or to the global, so I don't understand why you are willing to contemplate one but not the other.

as I disagree with your weighting as you are not taking into account the existing ecosystem and the feasibility of migration.

Migrating from using Reflect.getMetadata to ctx.tsDesignInfo or to globalThis.__tsMetadata seems like it would be just as feasible as migrating to metadata.tsDesignInfo. What is the relevant difference here?

@rbuckton
Copy link

rbuckton commented Mar 8, 2023

Wait, I'm confused. This entire discussion has been about TS adding TS-specific runtime behavior - injecting metadata derived from the types into the runtime semantics.

That's somewhat different from hijacking the entire decoration mechanism to replace an immutable context with one that is mutable, as was suggested in one example. We're also talking about an existing behavior with TypeScript that we cannot completely abandon, lest our users are unable to migrate to native decorator semantics.

Such patching would also result in decorators that only work when used in TypeScript, resulting in further bifurcation of the ecosystem.

No, you can still make such a decorator work when used in raw JS, as I showed above.

That is not equivalent, as design-time metadata is currently available to more than just an individual decoration, but stored with metadata associated with the entire class so that it can be also read later. To actually implement that mechanism using the approach you suggested would be unmanageable, and its results unreadable (which matters quite often when debugging).

But since "combining multiple pieces of code with different authors who aren't coordinating" is one of the fundamental things a programming language ought to enable, introducing a feature whose natural use will prevent that is bad.

Yet a frozen metadata object does not facilitate coordination, it expressly forbids it. What you are talking about isn't coordination, its isolation. You can have isolation with a mutable object, but I can't have data sharing with a frozen one.

This isn't something which can be fixed by a lint rule, because it's a coordination problem.

I disagree. A type-aware lint rule, such as one using the typescript-eslint plugin, would be able to catch this.

Coordination problems are precisely those which no one notices when building their own component - they only become a problem when other people start trying to combine that component with others. Avoiding coordination problems requires the language to be designed in such a way that they do not arise.

Unfortunately, ECMAScript was not designed in such a way to begin with. While I'm not against taking a principled stand for a specific feature when the opportunity arises, doing so here would be to the detriment of a community of existing users who would be unable to migrate. These are the people who we collected feedback from, and who drove us to seek inclusion of this feature in the language.

I'm still not understanding why it's not feasible - I've sketched several solutions which seem to me to work fine.

I've provided counterarguments to each solution as to why they are not feasible. Each would require a level of complexity so as to either make the emit unreadable, or to introduce a runtime performance penalty that is tantamount to being unusable. That has been an acceptable compromise for down-level emit, as it can often be complicated to replicate newer language semantics in older runtimes. This, however, is a feature that we would like to be able to use without significant downleveling, so it does not justify the cost.

Perhaps it's a difference of opinion over what "new non-compliant runtime behavior" means? As far as I can tell adding a new field to the metadata object is exactly the same level of non-compliant as adding a new field to the context object or to the global, so I don't understand why you are willing to contemplate one but not the other.

As I've said before, it is not as simple as "adding a new field to the context object". Adding a new field to a global is something we are expressly trying to avoid as it makes such emit unusable in SES code post-lockdown(). Also, I'm not clear on what you mean by "willing to contemplate one but not the other". I'm trying to avoid adding a new field to either the context or global, aside from the contents of the metadata object itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests