Skip to content
This repository has been archived by the owner on Jan 19, 2019. It is now read-only.

Decorator output is inconsistent #250

Closed
vjeux opened this issue May 4, 2017 · 7 comments
Closed

Decorator output is inconsistent #250

vjeux opened this issue May 4, 2017 · 7 comments
Assignees
Labels

Comments

@vjeux
Copy link

vjeux commented May 4, 2017

In this example, the decorator is not wrapped in a Decorator node:

class Person {
  @inject leftLeg: Leg;
}

==>

node.decorators = [ {type: 'Identifier'} ];

In this example, it is wrapped in a TSDecorator node, instead of Decorator node:

@dec enum E {}

==>

node.decorators = [ {type: 'TSDecorator', expression: {type: 'Identifier'}} ];

It would be nice if decorators arrays was always wrapped in a Decorator node (without TS prefix). This way it would work out of the box with babylon decorators.

I have a workaround in prettier, but I'd rather it be fixed inside of this project: prettier/prettier#1509

@soda0289
Copy link
Member

soda0289 commented May 4, 2017

Thanks for the report.
We can make this change. We should be following the estree spec:
https://github.com/estree/estree/blob/master/experimental/decorators.md

Typescript also allows decorators on method parameters but should still be using the same node type in all cases.

@vjeux
Copy link
Author

vjeux commented May 4, 2017

Thanks for being super responsive!

@JamesHenry
Copy link
Member

Nice to see you over this side @vjeux 😉

Just FYI @dec enum E {} is semantically invalid TypeScript, there are currently no examples of decorators outside of a class context in TS. But regardless, I agree that the AST should be consistent, even when the semantics are incorrect. The raw TypeScript AST definitely is in this case.

I'll submit a PR for this tomorrow.

@JamesHenry JamesHenry self-assigned this May 4, 2017
@vjeux
Copy link
Author

vjeux commented May 4, 2017

We're running prettier on all the typescript tests and looking at where it crashes. So expect a lot of crazy edge cases uncovered!

@vjeux
Copy link
Author

vjeux commented May 23, 2017

vscode uses a lot of decorators and right now prettier is crashing because of the inconsistency fyi.

vjeux added a commit to vjeux/prettier that referenced this issue May 23, 2017
See this issue for creating the same decorators as babylon ( eslint/typescript-eslint-parser#250 ), in the meantime, this is making a ton of files in vscode crash. So we can just support both :)
vjeux added a commit to vjeux/prettier that referenced this issue May 23, 2017
See this issue for creating the same decorators as babylon ( eslint/typescript-eslint-parser#250 ), in the meantime, this is making a ton of files in vscode crash. So we can just support both :)
vjeux added a commit to prettier/prettier that referenced this issue May 23, 2017
See this issue for creating the same decorators as babylon ( eslint/typescript-eslint-parser#250 ), in the meantime, this is making a ton of files in vscode crash. So we can just support both :)
@JamesHenry
Copy link
Member

@vjeux Finally got to this, sorry for the delay #293

@vjeux
Copy link
Author

vjeux commented May 25, 2017

Nice! Thanks. I added a workaround inside of prettier for now, but i'd love to get rid of it :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants