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

Expose design-time type information in TC39 decorator metadata when emitDecoratorMetadata: true #57533

Open
6 tasks done
artberri opened this issue Feb 25, 2024 · 7 comments Β· May be fixed by #58101
Open
6 tasks done

Expose design-time type information in TC39 decorator metadata when emitDecoratorMetadata: true #57533

artberri opened this issue Feb 25, 2024 · 7 comments Β· May be fixed by #58101
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@artberri
Copy link

πŸ” Search Terms

decorators, metadata, emitDecoratorMetadata, "TC39 decorators", "type information", reflect-metadata

βœ… Viability Checklist

⭐ Suggestion

I want to be able to have design-time type information at runtime after the implementation of the new TC39 decorators system.

After the implementation of the TC39 decorators in Typescript 5.0 the ability to emit design-time type information has been lost unless we keep using the experimental (legacy) decorators system.

The suggestion is to allow to use emitDecoratorMetadata: true with experimentalDecorators: false. The idea is to expose the same metadata that it was being exposed to Reflect.metadata("design:type", type) with the legacy decorators, to the new metadata system proposed by TC39 that has been implemented in Typescript 5.2.

So, instead of using the obsolete reflect-metadata like Reflect.getMetadata('design:type', target, property), we can get the same info by doing something like SomeClass[Symbol.metadata]["design:type"].

πŸ“ƒ Motivating Example

After the decorators change, many existing and widely used libraries will have problems implementing their behaviors without type information at runtime: type-graphql, inversify, Angular 2,... or any other library which depends on reflect-metadata.

πŸ’» Use Cases

As explained in the "Motivating Example", there are several existing use cases that require some kind of design-time type information at runtime. My suggestion is to use more or less the same approach you used with the experimental decorators, but with the standard ones. But I will mostly agree to any other approach that makes this information available at runtime.

For now, the workaround is to keep using the experimental decorators and the reflect-metadata library (or compatible). But I think it is worth giving the standard ones the same ability to expose this info.

@RyanCavanaugh
Copy link
Member

This could be implemented without emitting different JS based on the types of the expressions

Can you give some examples of how this would work while maintaining this invariant?

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. labels Mar 1, 2024
@bcheidemann
Copy link

bcheidemann commented Apr 3, 2024

This could be implemented without emitting different JS based on the types of the expressions

Can you give some examples of how this would work while maintaining this invariant?

@RyanCavanaugh I believe the invariant would be maintained as the feature requires emitting different JS based on the types of declarations, not expressions:

class Example {
    property: string; // This is a property declaration, not an expression
    method(param: string): string { // This is a method declaration, not an expression
        return "";
    }
}

Please let me know if I've misunderstood btw! :)

@bcheidemann
Copy link

I've done some investigation into how this could be implemented. As far as I can tell, this would be a fairly trivial change to implement. I was able to get it working for a few basic examples with some pretty minimal changes:

diff --git a/src/compiler/factory/emitHelpers.ts b/src/compiler/factory/emitHelpers.ts
index 0937915e75..9e6e06a1d5 100644
--- a/src/compiler/factory/emitHelpers.ts
+++ b/src/compiler/factory/emitHelpers.ts
@@ -736,7 +736,9 @@ export const metadataHelper: UnscopedEmitHelper = {
     priority: 3,
     text: `
             var __metadata = (this && this.__metadata) || function (k, v) {
-                if (typeof Reflect === "object" && typeof Reflect.metadata === "function") return Reflect.metadata(k, v);
+                return function (_, c) {
+                    c.metadata[k] = v;
+                }
             };`,
 };
 
diff --git a/src/compiler/program.ts b/src/compiler/program.ts
index c46e910a53..dd9f29cb1c 100644
--- a/src/compiler/program.ts
+++ b/src/compiler/program.ts
@@ -4357,13 +4357,6 @@ export function createProgram(rootNamesOrOptions: readonly string[] | CreateProg
             }
         }
 
-        if (
-            options.emitDecoratorMetadata &&
-            !options.experimentalDecorators
-        ) {
-            createDiagnosticForOptionName(Diagnostics.Option_0_cannot_be_specified_without_specifying_option_1, "emitDecoratorMetadata", "experimentalDecorators");
-        }
-
         if (options.jsxFactory) {
             if (options.reactNamespace) {
                 createDiagnosticForOptionName(Diagnostics.Option_0_cannot_be_specified_with_option_1, "reactNamespace", "jsxFactory");
diff --git a/src/compiler/transformers/ts.ts b/src/compiler/transformers/ts.ts
index f7af3ceb8e..9f51a89ca7 100644
--- a/src/compiler/transformers/ts.ts
+++ b/src/compiler/transformers/ts.ts
@@ -1112,7 +1112,6 @@ export function transformTypeScript(context: TransformationContext) {
      */
     function getTypeMetadata(node: Declaration, container: ClassLikeDeclaration) {
         // Decorator metadata is not yet supported for ES decorators.
-        if (!legacyDecorators) return undefined;
         return USE_NEW_TYPE_METADATA_FORMAT ?
             getNewTypeMetadata(node, container) :
             getOldTypeMetadata(node, container);

Of course, in order to still support legacy decorators, some additional changes would be required. But these would likely also be straightforward.

@bcheidemann
Copy link

@RyanCavanaugh I've raised a draft PR to serve as a proposal for how this could be implemented, and to serve as a PoC. Please let me know if there's anything else I can do to help progress this issue :)

@artberri
Copy link
Author

artberri commented Apr 7, 2024

This could be implemented without emitting different JS based on the types of the expressions

Can you give some examples of how this would work while maintaining this invariant?

Aside from what @bcheidemann said, I checked this box because it won't emit different JS than the one emitted by the legacy decorators. So, if this invariant was met by the previous implementation, it would also be met by the new one.

@RyanCavanaugh, if you interpret this to mean that mimicking the previous behaviour does not preserve it, I will uncheck it, but I still think that this should be implemented to preserve decorator metadata support, as a lot of libraries depend on it.

BTW, thank you very much @bcheidemann for your support and the proposal PR.

@fatcerberus
Copy link

fatcerberus commented Apr 8, 2024

So, if this invariant [no different JS based on types] was met by the previous implementation, it would also be met by the new one.

fwiw that invariant isn't met by the legacy implementation - it falls under the grandfather clause because it was done before the "no type-directed emit" rule was instituted. This is similar to how

This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)

is violated by enum and namespace, for the same reasons.

@bcheidemann
Copy link

Thanks for clarifying @fatcerberus. Do you know what the thinking is on this... since it was supported for legacy decorators, is it likely to continue being supported with the new ES decorators in some capacity despite violating this invariant?

Also could you confirm how exactly the invariant is violated by this feature? It might be helpful to have some explanation of this if the feature ends up being rejected on these grounds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants