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

Implement decorator metadata proposal #53461

Closed
5 tasks done
justinfagnani opened this issue Mar 23, 2023 · 17 comments Β· Fixed by #54657
Closed
5 tasks done

Implement decorator metadata proposal #53461

justinfagnani opened this issue Mar 23, 2023 · 17 comments Β· Fixed by #54657
Assignees
Labels
Committed The team has roadmapped this issue Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript

Comments

@justinfagnani
Copy link

justinfagnani commented Mar 23, 2023

Suggestion

Implement decorator metadata

πŸ” Search Terms

decorator metadata

βœ… Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

Now that decorators are shipping in TypeScript and decorator metadata has reached consensus on the approach needed for it to proceed to Stage 3, it seems like it may be time to implement metadata.

πŸ“ƒ Motivating Example

There are examples on the metadata repo, but a quick one is storing information about a class member to be used by the class's base class. Here we generate a custom element's observedAttributes by decorating fields:

// Library code:
const attributes = new WeakMap<typeof BaseElement, Array<string>>();
class BaseElement {
  static get observedAttributes() {
    // real code would get attributes from super-classes too
    return attributes.get(this[Symbol.metadata]);
  }
}
const attribute = (_, {name, metadata}) => {
  attributes.set(metadata, (attributes.get(metadata) ?? []).push(name));
}

// Usage:
class MyElement extends BaseElement {
  @attribute
  foo?: string;
}

πŸ’» Use Cases

There are many use cases in the various decorators repositories and documents. Generally any time that some other API besides the decorated one needs information about the decorated items, you will need metadata.

It's worth noting that many libraries can't use decorators until metadata is implemented.

@MartinJohns
Copy link
Contributor

MartinJohns commented Mar 23, 2023

[x] This feature would agree with the rest of TypeScript's Design Goals.

I don't know the current position of the team on this, but it does violate non-goal 5. The current experimental metadata support comes from the early dark times before this design goal document.

Nevermind, you refer to an actual ES proposal. But that is only stage 2, usually the team waits for at least stage 3.

@justinfagnani
Copy link
Author

@MartinJohns

But that is only stage 2, usually the team waits for at least stage 3.

Yes, but like I mentioned the metadata proposal just got consensus on the approach to enable it to go to Stage 3 very soon. An issue like this can be used to track some work even before that work could be done or shipped.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Committed The team has roadmapped this issue labels Mar 23, 2023
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 5.2.0 milestone Mar 23, 2023
@DanielRosenwasser
Copy link
Member

Tentatively scheduling for TypeScript 5.2. Luckily, that's not too far off anyway.

@EisenbergEffect
Copy link

EisenbergEffect commented Mar 30, 2023

Please. Please. Please. Please. Please. Please. Please.

@DanielRosenwasser I will send your team a cake. Or, if you like other tasties, let me know. Clearly I'm not above bribery when it comes to decorator metadata 🀣

/cc @chrisdholt

@EisenbergEffect
Copy link

Stage 3 is official!!! 🎊

@rbuckton
Copy link
Member

rbuckton commented May 19, 2023

The downlevel emit for this is fairly simple, and should work with the existing helpers. When I wrote the __esDecorate, the intent was for it to easily support metadata a well.

given

@dec
class C {
}

we used to emit

let C = (() => {
    let _classDecorators = [dec];
    let _classDescriptor;
    let _classExtraInitializers = [];
    let _classThis;
    var C = class {
        static {
            __esDecorate(null, _classDescriptor = { value: this }, _classDecorators, { kind: "class", name: this.name }, null, _classExtraInitializers);
            C = _classThis = _classDescriptor.value;
            __runInitializers(_classThis, _classExtraInitializers);
        }
    };
    return C = _classThis;
})();

But in the future, we will emit this instead:

let C = (() => {
    let _classDecorators = [dec];
    let _classDescriptor;
    let _classExtraInitializers = [];
    let _classThis;
    var C = class {
        static {
            const metadata = typeof Symbol === "function" && Symbol.metadata ? Object.create(null) : undefined;
            __esDecorate(null, _classDescriptor = { value: this }, _classDecorators, { kind: "class", name: this.name, metadata }, null, _classExtraInitializers);
            C = _classThis = _classDescriptor.value;
            if (metadata) Object.defineProperty(C, Symbol.metadata, { configurable: true, writable: true, value: metadata });
            __runInitializers(_classThis, _classExtraInitializers);
        }
    };
    return C = _classThis;
})();

Metadata availability will be conditional on the presence of Symbol.metadata, so as not to break existing users.

@EisenbergEffect
Copy link

YES!!!! Awesome work everyone. @DanielRosenwasser please reach out to me on twitter, linkedin, etc. I really would like to thank the team somewhow.

@justinfagnani
Copy link
Author

justinfagnani commented Jun 26, 2023

@rbuckton would #54657 be in the latest nightly? I'm trying to use metadata, and while it's in the typings, it appears to always be undefined: Playground

edit to add: I don't see a way to choose libs in the playground, but I did add 'esnext.decorators' locally, thinking that that might add the definition for Symbol.metadata, but context.metadata is still undefined.

@rbuckton
Copy link
Member

Have you polyfilled Symbol.metadata? TypeScript does not polyfill runtime features, so you may need to add something like the following to use it:

(Symbol as any).metadata ??= Symbol.for("Symbol.metadata");

Also, you can use // @lib: dom,esnext in the playground to choose different libs.

@justinfagnani
Copy link
Author

I just figured that out, thanks!

@jakearchibald
Copy link

@rbuckton I got caught out by metadata being undefined too. Would it make more sense to leave it defined even if Symbol.metadata is missing?

The current developer experience is:

  1. Read the metadata docs
  2. Try it in TypeScript
  3. Try to set context.metadata[key]
  4. context.metadata is undefined, wtf??

Whereas if context.metadata was still defined, it's:

  1. Read the metadata docs
  2. Try it in TypeScript
  3. Set context.metadata[key]
  4. Try to get the metadata from the class
  5. Symbol.metadata is undefined, wtf??

With the latter case, you run into the actual problem, not a side-effect of the problem.

@justinfagnani
Copy link
Author

I agree that seeing an error that Symbol.metadata is undefined would be a lot more clear - I initially thought that the TS metadata implementation had a bug).

I think changing the metadata initialization from this:

const _metadata = typeof Symbol === "function" && Symbol.metadata ? Object.create(null) : void 0;
// ...
if (_metadata) Object.defineProperty(this, Symbol.metadata, { enumerable: true, configurable: true, writable: true, value: _metadata });

to this:

const _metadata = Object.create(null);
// ...
if (typeof Symbol === "function" && Symbol.metadata) Object.defineProperty(this, Symbol.metadata, { enumerable: true, configurable: true, writable: true, value: _metadata });

Would ensure that context.metadata is defined, that it's shared across all decorator applications for a class, that metadata usage that's only within decorators (ie, as a way of sharing) just works, and that the error from Symbol.metadata being undefined is more understandable.

@jakearchibald
Copy link

Yep, +1 to that implementation

@rbuckton
Copy link
Member

If context.metadata is defined but isn't actually assigned to the class, it becomes more difficult for a decorator to test whether it can actually write metadata. This change wouldn't actually improve any use case, as any metadata you would attach would be lost.

Having context.metadata be undefined when not using a lib that has the symbol defined allows you to code defensively against environments that haven't shimmed Symbol.metadata. I'm not certain I'd be comfortable with this change.

I could see adding a documentation comment to the metadata property to explain the reason why it is missing.

@jakearchibald
Copy link

I guess if browsers shipped decorators without metadata, then context.metadata would be undefined.

Yeah, improved documentation seems reasonable.

@rbuckton
Copy link
Member

rbuckton commented Sep 28, 2023

The current approach also allows you to write the following shim in your code to enable metadata downlevel, if necessary:

// metadataShim.ts
export {};
declare global {
  interface SymbolConstructor {
    readonly metadata: unique symbol;
  }
}
Symbol.metadata ??= Symbol.for("Symbol.metadata");

// yourCode.ts
import "./metadataShim.js";
...

The above code would also remove the | undefined from context.metadata due to the conditional type used to define it.

@justinfagnani
Copy link
Author

Having context.metadata be undefined when not using a lib that has the symbol defined allows you to code defensively against environments that haven't shimmed Symbol.metadata. I'm not certain I'd be comfortable with this change.

This is a really good point. We already do this in Lit 3.0 with a dev-mode warning when metadata is undefined telling people that they need to upgrade their compiler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committed The team has roadmapped this issue Fix Available A PR has been opened for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants