-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix(ast-spec): remove more invalid properties #6243
fix(ast-spec): remove more invalid properties #6243
Conversation
Thanks for the PR, @JoshuaKGoldberg! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -8,26 +8,6 @@ Object { | |||
"body": Array [ | |||
Object { | |||
"computed": false, | |||
"export": undefined, | |||
"initializer": Object { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm - i wonder if we should throw on this specific case?
prettier currently supports it, but other parsers (babel) throw here.
If we don't throw, I believe prettier will just delete the code silently when it formats.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bradzacher @fisker I went ahead and added errors for the other @deprecated
node properties in typescript.d.ts
(TIL!):
interface PropertySignature {
/** @deprecated A property signature cannot have an initializer */
readonly initializer?: Expression | undefined;
}
interface PropertyAssignment {
/** @deprecated A property assignment cannot have a question token */
readonly questionToken?: QuestionToken | undefined;
/** @deprecated A property assignment cannot have an exclamation token */
readonly exclamationToken?: ExclamationToken | undefined;
}
interface ShorthandPropertyAssignment {
/** @deprecated A shorthand property assignment cannot have modifiers */
readonly modifiers?: NodeArray<Modifier> | undefined;
/** @deprecated A shorthand property assignment cannot have a question token */
readonly questionToken?: QuestionToken | undefined;
/** @deprecated A shorthand property assignment cannot have an exclamation token */
readonly exclamationToken?: ExclamationToken | undefined;
}
interface FunctionTypeNode {
/** @deprecated A function type cannot have modifiers */
readonly modifiers?: NodeArray<Modifier> | undefined;
}
Do you think this is reasonable for Prettier too? I'd be interested in sending a PR over if so, I haven't actually contributed to Prettier yet!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Saving us from checking those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fisker I poked around in Prettier-land and couldn't figure out if there was anything else for me to change there. I'll defer to you. If you end up filing an issue in Prettier I'd love to tackle it. But it's a bit much for me on my own. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The invalid modifier check is here in this file https://github.com/prettier/prettier/blob/next/src/language-js/parse/postprocess/typescript.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @fisker / @sosukesuzuki for the prettier POV. We're removing bits of the AST that shouldn't be there in our next major. All the things we are removing are bad code that should probably be syntax errors, but aren't for because #just-ts-things. From prettier's POV - would it be better if: WDYT? |
We already check lot's of errors in our next branch, so we can test those properties, it doesn't really matter. It will be nice to throw errors here, I choose c). Anyway, it's always good to have a choice, so if not much trouble d) is best for now (we may still need release Prettier 2.x), but after Prettier v3, we definitely don't want hide syntax errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's kill them ALL!
This comment was marked as outdated.
This comment was marked as outdated.
@JoshuaKGoldberg I think we need to be a little stricter here and explicitly error if we encounter this weird bit of syntax. The issue with just ignoring it is that it'll potentially have downstream effects on ESLint fixers that act on raw source code and could lead to a fixer introducing a syntax error. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## v6 #6243 +/- ##
==========================================
- Coverage 87.15% 87.10% -0.06%
==========================================
Files 362 362
Lines 12459 12470 +11
Branches 3688 3688
==========================================
+ Hits 10859 10862 +3
- Misses 1253 1261 +8
Partials 347 347
Flags with carried forward coverage won't be shown. Click here to find out more.
|
BREAKING CHANGE:
Removes properties from
ast-spec
and thereforetypescript-estree
PR Checklist
Overview
Removes the additional properties listed by @bradzacher in #4759.