Skip to content

Throw errors for non-class decorators #185

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

Closed
ikatyang opened this issue Feb 1, 2019 · 8 comments · Fixed by #211
Closed

Throw errors for non-class decorators #185

ikatyang opened this issue Feb 1, 2019 · 8 comments · Fixed by #211
Assignees
Labels
package: typescript-estree Issues related to @typescript-eslint/typescript-estree triage Waiting for team members to take a look

Comments

@ikatyang
Copy link
Contributor

ikatyang commented Feb 1, 2019

What code were you trying to parse?

@dec()
const a = 1

What did you expect to happen?

Throw an error.

What actually happened?

No decorators attached nor errors were thrown.

Versions

package version
@typescript-eslint/typescript-estree 1.1.0
TypeScript 3.2.2
node X.Y.Z
npm X.Y.Z

Original post: prettier/prettier#5823

@ikatyang ikatyang added package: typescript-estree Issues related to @typescript-eslint/typescript-estree triage Waiting for team members to take a look labels Feb 1, 2019
@armano2
Copy link
Collaborator

armano2 commented Feb 1, 2019

we are reporting error for decorators:

case 1206: // ts 3.2 "Decorators are not valid here."

you should enable errorOnTypeScriptSyntacticAndSemanticIssues option

@JamesHenry
Copy link
Member

We need to be careful recommending that to Prettier at this stage @armano2 - there’s a major difference in performance to think about right now.

@JamesHenry
Copy link
Member

We should measure for the Prettier case specifically, there’s a big user base to consider

@JamesHenry
Copy link
Member

I'm working on this now

@JamesHenry
Copy link
Member

JamesHenry commented Feb 5, 2019

I've worked on this a bit and we already have precedent for adding decorators on nodes which don't allow them, so I am thinking we should just do that here too to solve them problem for now.

Enabling the diagnostics is expensive, it looks like it's around 30% slower for real world usage of Prettier, because it otherwise doesn't need a lot of the Program architecture.

That might be fine in many cases, might not in others, hard to tell. But given this specific issue, and the fact we already have precedent for this, I think handling it specifically is probably the best way to go.

@JamesHenry
Copy link
Member

In the future, it would be really nice if there were a way to cheaply report diagnostics on a single SourceFile, rather than needing a Program cc @typescript-eslint/typescript-team

@JamesHenry
Copy link
Member

JamesHenry commented Feb 5, 2019

PR is here: #211

@uniqueiniquity
Copy link
Contributor

RE: the diagnostics on a single sourcefile, they're technically there, but not exposed in the API. I'll ask around tomorrow to see if that's a safe thing to use/depend on or not

@typescript-eslint typescript-eslint locked as resolved and limited conversation to collaborators Feb 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: typescript-estree Issues related to @typescript-eslint/typescript-estree triage Waiting for team members to take a look
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants