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

Fixes #420 Decorate exported class #425

Merged
merged 2 commits into from
Mar 2, 2023
Merged

Fixes #420 Decorate exported class #425

merged 2 commits into from
Mar 2, 2023

Conversation

STRd6
Copy link
Contributor

@STRd6 STRd6 commented Mar 2, 2023

This allows decorators in TS "before export" and TC39 "after export" positions and doesn't try to do anything smart about it yet.

I added another state flag for when we are inside Decorators to prevent @@decorator class ... from being treated as an implicit call passing a class expression to the decorator function. If someone really wants to do that they'll need to use parentheses and be explicit.

@STRd6 STRd6 temporarily deployed to build March 2, 2023 00:32 — with GitHub Actions Inactive
@@ -5349,6 +5349,9 @@ TypeDeclaration
( Export _? )? ( Declare _? ) TypeLexicalDeclaration -> { ts: true, children: $0 }
( Export _? )? ( Declare _? )? TypeDeclarationRest -> { ts: true, children: $0 }

# NOTE: TS Exported classes can have decorators before export but not after
Decorators ( Export _? ) !Decorators ClassDeclaration -> { ts: true, children: $0 }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this should be ts: true. Exported classes should be in JS code too, just like other decorated classes.

Related, why is this here and not in ExportDeclaration?

Ah, I see, this type of decorating is inconsistent with the TC39 decorator proposal...

@STRd6 STRd6 temporarily deployed to build March 2, 2023 01:28 — with GitHub Actions Inactive
@edemaine
Copy link
Collaborator

edemaine commented Mar 2, 2023

Could you explain (maybe with a test case) why we need another flag here? Oh, was this a consequence of extending to CallExpression in a previous PR? Kind of surprised any tests passed...

I could see forbidding any implicit call with a class expression... But I guess it's nice to preserve it.

@STRd6
Copy link
Contributor Author

STRd6 commented Mar 2, 2023

Yeah, the idea was to keep allowing fn class ... as an implicit application generally but disallow it in inline decorators.

@STRd6 STRd6 merged commit 082fd34 into main Mar 2, 2023
@STRd6 STRd6 deleted the export-decorators branch March 2, 2023 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants