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

feat(53461): Implement decorator metadata proposal #54657

Merged
merged 12 commits into from
Jun 23, 2023

Conversation

a-tarasyuk
Copy link
Contributor

@a-tarasyuk a-tarasyuk commented Jun 15, 2023

Fixes #53461

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Jun 15, 2023
@jakebailey
Copy link
Member

FYI @rbuckton

Copy link
Member

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add Symbol.metadata to the libs, probably under lib/esnext.decorators.d.ts

In lib/decorators.d.ts we can make context.metadata conditional using an approach like this:

// lib.esnext.decorators.d.ts
interface Symbol {
  readonly metadata: unique symbol;
}

// lib.decorators.d.ts
...

type DecoratorMetadata = typeof globalThis extends { Symbol: { readonly metadata: symbol } } ? object : object | undefined;

interface ClassDecoratorContext<...> {
  ...
  readonly metadata: DecoratorMetadat;
  ...
}

...

interface ClassMethodDecoratorContext<...> {
  ...
  readonly metadata: DecoratorMetadat;
  ...
}

... etc. ...

src/compiler/factory/emitHelpers.ts Outdated Show resolved Hide resolved
src/compiler/transformers/esDecorators.ts Outdated Show resolved Hide resolved
src/compiler/transformers/esDecorators.ts Outdated Show resolved Hide resolved
src/compiler/transformers/esDecorators.ts Outdated Show resolved Hide resolved
src/compiler/transformers/esDecorators.ts Outdated Show resolved Hide resolved
src/compiler/transformers/esDecorators.ts Outdated Show resolved Hide resolved
src/compiler/transformers/esDecorators.ts Outdated Show resolved Hide resolved
src/compiler/transformers/esDecorators.ts Outdated Show resolved Hide resolved
src/compiler/transformers/esDecorators.ts Outdated Show resolved Hide resolved
src/compiler/transformers/esDecorators.ts Outdated Show resolved Hide resolved
@rbuckton
Copy link
Member

Ideally, we can take this for the 5.2 beta which has a deadline of this Friday. Let me know if you need me to help with any part of this PR to get it in this week.

@a-tarasyuk
Copy link
Contributor Author

@rbuckton Thank you so much for taking the time to review this PR 👍🏻. I have added the esnext.decorators library and made changes related to types and factory helpers. I have noticed that the super call is transformed into Reflect.get in classFields. I'm not entirely sure if this is okay, I will check it tomorrow…

Copy link
Member

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would also help to have evaluation tests that run the some of the examples used in the esDecoratorMetadata tests and verify the expected output (see src\testRunner\unittests\evaluation\esDecorators.ts).

To make that work you would need to shim Symbol.metadata for the evaluation tests, which you can do in src\harness\evaluatorImpl.ts:20. See
https://github.com/microsoft/TypeScript/pull/54505/files?show-viewed-files=true&file-filters%5B%5D=#diff-3b6817a4cdcf825c75e6f9af6cad21356ee6a7cd35bb5d3c92eef8e0985ce910R20-R33 for an example.

src/lib/decorators.d.ts Show resolved Hide resolved
src/lib/esnext.decorators.d.ts Show resolved Hide resolved
src/compiler/transformers/esDecorators.ts Outdated Show resolved Hide resolved
src/compiler/transformers/esDecorators.ts Outdated Show resolved Hide resolved
…uards to avoid crashes caused by incorrect base prototypes
@a-tarasyuk a-tarasyuk changed the title [WIP] feat(53461): Implement decorator metadata proposal feat(53461): Implement decorator metadata proposal Jun 22, 2023
@a-tarasyuk a-tarasyuk marked this pull request as ready for review June 22, 2023 17:45
@a-tarasyuk a-tarasyuk requested a review from rbuckton June 22, 2023 17:47
@rbuckton
Copy link
Member

Some tests are failing and there are merge conflicts. Can you verify baselines and update with main?

@a-tarasyuk
Copy link
Contributor Author

@rbuckton I've merged changes from the main

@rbuckton
Copy link
Member

It looks like one of your evaluation tests is failing.

@rbuckton
Copy link
Member

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2023

Heya @rbuckton, I've started to run the tarball bundle task on this PR at 3515e58. You can monitor the build here.

@a-tarasyuk
Copy link
Contributor Author

It looks like one of your evaluation tests is failing.

Yes. It due to #54657 (comment)

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 22, 2023

Hey @rbuckton, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/155600/artifacts?artifactName=tgz&fileId=907D5787AC0EB0691771857360E56A79917C1534F3C1E3FF9382E73F0CF73DCE02&fileName=/typescript-5.2.0-insiders.20230622.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.2.0-pr-54657-9".;

@rbuckton
Copy link
Member

rbuckton commented Jun 22, 2023

We should also add this to esnext.decorators.d.ts:

interface Function {
  [Symbol.metadata]?: object | null;
}

Sorry, we should add it to src/lib/decorators.d.ts instead, per my other suggestion.

src/lib/decorators.d.ts Outdated Show resolved Hide resolved
@rbuckton
Copy link
Member

This is looking fairly close to ready, once the baselines are reviewed and accepted I'll hopefully be able to take a final pass and approve.

@a-tarasyuk
Copy link
Contributor Author

a-tarasyuk commented Jun 23, 2023

@rbuckton What are your thoughts on moving the following types from decorators.d to esnext.decorators.d?

interface Function {
    [Symbol.metadata]: DecoratorMetadata | null;
}

@rbuckton
Copy link
Member

It shouldn't be optional since it will already get the | undefined from the DecoratorMetadata type with the definition I suggested. Placing it in esnext.decorators makes sense given the dependence on Symbol.

@a-tarasyuk
Copy link
Contributor Author

oke, I've moved it to esnext.decorators.d.

@a-tarasyuk a-tarasyuk requested a review from rbuckton June 23, 2023 06:14
@rbuckton
Copy link
Member

It shouldn't be optional since it will already get the | undefined from the DecoratorMetadata type with the definition I suggested. Placing it in esnext.decorators makes sense given the dependence on Symbol.

I should have added more detail to this, but it was late. The main reason to not make it an optional property of Function is that we wouldn't be able to make it not optional in esnext (or es2024, etc.). There is a way to do that, but it would have been more complex and employ the same mechanism as DecoratorMetadata already does, i.e.:

type FunctionMetadata =
    typeof globalThis extends { Symbol: { readonly metadata: symbol } } ? {
        [Symbol.metadata]: DecoratorMetadataObject | null;
    } : {
        [Symbol.metadata]?: DecoratorMetadataObject | null;
    };

interface Function extends FunctionMetadata {
}

But I think the approach of just using DecoratorMetadata—which already includes this conditional—and leaving the property as non-optional is fine.

@rbuckton rbuckton merged commit bc91920 into microsoft:main Jun 23, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement decorator metadata proposal
5 participants