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

Decorators normative changes #52582

Merged
merged 7 commits into from
Feb 9, 2023
Merged

Decorators normative changes #52582

merged 7 commits into from
Feb 9, 2023

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Feb 2, 2023

These changes reflect the consensus on normative changes for context.access in decorators, and allowing decorators before OR after export.

Fixes #52540
Fixes #52578

@jakebailey
Copy link
Member

Do we have a test that verifies that you can't have decorators both before and after export? I tried looking but didn't see any or a message relating to that.

// @filename: file5.ts

// ok
export default @dec class C5 {}
Copy link
Member

Choose a reason for hiding this comment

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

What about @dec export default @dec and @dec export @dec?

Does this PR fix #52578?

Copy link
Member Author

@rbuckton rbuckton Feb 3, 2023

Choose a reason for hiding this comment

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

Does this PR fix #52578?

Yes, I'll add it.

@jakebailey
Copy link
Member

Also, do we have tests which test emit for a potential es2023 which includes this syntax? I know you had mentioned that we need extra logic to make sure we emit it into the right place, but I couldn't find a test which shows that we do that.

@rbuckton
Copy link
Member Author

rbuckton commented Feb 3, 2023

Do we have a test that verifies that you can't have decorators both before and after export? I tried looking but didn't see any or a message relating to that.

I thought I had, but I will add one. We do report an error currently, but the error is Declaration Expected because we expect that class follows immediately after @dec export. I may need to make that a bit more flexible so that we can report a better error.

@rbuckton
Copy link
Member Author

rbuckton commented Feb 3, 2023

Also, do we have tests which test emit for a potential es2023 which includes this syntax? I know you had mentioned that we need extra logic to make sure we emit it into the right place, but I couldn't find a test which shows that we do that.

For --target esnext, you can see the emit for both cases in the outputs for esDecorators-classDeclaration-exportModifier.2:

  • file1.js emits @dec before export
  • file2.js emits @dec before export default
  • file4.js emits @dec after export
  • file5.js emits @dec after export default

@jakebailey
Copy link
Member

Doh, I didn't find it because it was a test that only tested esnext (I was looking at filenames).

@rbuckton
Copy link
Member Author

rbuckton commented Feb 3, 2023

@DanielRosenwasser, your feedback on the diagnostic messages would be appreciated as well.

@rbuckton
Copy link
Member Author

rbuckton commented Feb 3, 2023

I'm also working on tightening up the types for context.access, making them readonly fields instead of methods. That will likely have an impact on inference due to changes in variance, so I'll put up a separate PR for those after I've finished some tests.

@DanielRosenwasser
Copy link
Member

Let's also add a test for the abstract keyword mixed with

  • decorators after export, before `abstract
  • decorators after export, after `abstract

@rbuckton rbuckton force-pushed the decorators-normative-changes branch from e9817ff to 9ba73ed Compare February 3, 2023 19:57
@rbuckton
Copy link
Member Author

rbuckton commented Feb 3, 2023

Let's also add a test for the abstract keyword mixed with

  • decorators after export, before `abstract
  • decorators after export, after `abstract

Done.

export default @dec abstract class C12 {}

==== tests/cases/conformance/esDecorators/classDeclaration/file13.ts (2 errors) ====
// error
Copy link
Member

Choose a reason for hiding this comment

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

aside - I don't like the // ok vs. // error comments because it is possible for them to change over time. I would prefer the baselines be the source of truth.

Copy link
Member

Choose a reason for hiding this comment

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

I'm torn, because on one hand, the baselines are the source of truth, but on the other, if I didn't write the baselines, I don't have any indication as to what the original intended behavior was...

Copy link
Member Author

Choose a reason for hiding this comment

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

They were, at the very least, useful to verify that I was getting errors where I expected them when running the tests initially. I can remove the comments, or make them more detailed to explain why an error is expected at these locations.

@rbuckton
Copy link
Member Author

rbuckton commented Feb 6, 2023

We may want to wait until pzuraq/ecma262#4 has merged into the official ecma262 PR for decorators before we merge this.

@rbuckton
Copy link
Member Author

rbuckton commented Feb 9, 2023

pzuraq/ecma262#4 has merged. I will merge this shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
4 participants