Skip to content

Decorators normative changes#52582

Merged
rbuckton merged 7 commits intomainfrom
decorators-normative-changes
Feb 9, 2023
Merged

Decorators normative changes#52582
rbuckton merged 7 commits intomainfrom
decorators-normative-changes

Conversation

@rbuckton
Copy link
Contributor

@rbuckton rbuckton commented Feb 2, 2023 •
edited
Loading

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

Fixes #52540
Fixes #52578

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Feb 2, 2023
@jakebailey
Copy link
Member

jakebailey 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.

DanielRosenwasser
DanielRosenwasser reviewed Feb 3, 2023
View reviewed changes
.../conformance/esDecorators/classDeclaration/esDecorators-classDeclaration-exportModifier.2.ts
// @filename: file5.ts

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

@DanielRosenwasser DanielRosenwasser Feb 3, 2023

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
Contributor Author

@rbuckton rbuckton Feb 3, 2023 •
edited
Loading

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 jakebailey mentioned this pull request Feb 3, 2023
Closed
@jakebailey
Copy link
Member

jakebailey 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.

@rbuckton
Copy link
Contributor 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
Contributor 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

jakebailey commented Feb 3, 2023

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

@rbuckton
Copy link
Contributor Author

rbuckton commented Feb 3, 2023

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

@rbuckton
Copy link
Contributor 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

DanielRosenwasser 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

@rbuckton rbuckton force-pushed the decorators-normative-changes branch from e9817ff to 9ba73ed Compare February 3, 2023 19:57
@rbuckton
Copy link
Contributor 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.

DanielRosenwasser
DanielRosenwasser reviewed Feb 4, 2023
View reviewed changes
tests/baselines/reference/esDecorators-classDeclaration-exportModifier.2.errors.txt
export default @dec abstract class C12 {}

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

@DanielRosenwasser DanielRosenwasser Feb 4, 2023

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

@jakebailey jakebailey Feb 4, 2023

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
Contributor Author

@rbuckton rbuckton Feb 6, 2023

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
Contributor 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 rbuckton mentioned this pull request Feb 8, 2023
Merged
@rbuckton
Copy link
Contributor Author

rbuckton commented Feb 9, 2023

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

@rbuckton rbuckton merged commit 6c9792a into main Feb 9, 2023
@rbuckton rbuckton deleted the decorators-normative-changes branch February 9, 2023 19:48
@stormslowly stormslowly mentioned this pull request Feb 24, 2023
Merged
@yay yay mentioned this pull request Apr 12, 2023
Closed
@danielroe danielroe mentioned this pull request Jun 17, 2024
Merged
2 tasks
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DanielRosenwasser DanielRosenwasser DanielRosenwasser left review comments

@jakebailey jakebailey jakebailey approved these changes

@andrewbranch andrewbranch Awaiting requested review from andrewbranch

Assignees

@rbuckton rbuckton

Labels

Author: Team For Milestone Bug PRs that fix a bug with a specific milestone

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Preserve decorator ordering, remove restrictions on decorator-before-export in JavaScript files Revisit access member for decorators

4 participants

@rbuckton @jakebailey @DanielRosenwasser @typescript-bot