-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Fill optional AST properties when both estree and typescript parser plugin are enabled (Part 3) #17235
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
Fill optional AST properties when both estree and typescript parser plugin are enabled (Part 3) #17235
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/59091 |
fd2f90d
to
09de007
Compare
return; | ||
case "TSModuleDeclaration": | ||
node.declare ??= false; | ||
node.global ??= node.kind === "global"; |
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.
Since both we and they plan to remove it in the next major version, what do you think about not adding it?
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.
If we remove it before they release the next major, we will have to add those impacted tests to the failure set. However, test in the failure set does not really assert why the AST is different: So it may hide more AST differences than we expected.
For the time being, let's keep it for the compatibility with the current ts-eslint. We can remove it once they release a new major.
(node as Undone<N.ExportNamedDeclaration>).attributes = []; | ||
(node as Undone<N.ExportNamedDeclaration>).declaration = declaration; | ||
(node as Undone<N.ExportNamedDeclaration>).exportKind = "value"; | ||
(node as Undone<N.ExportNamedDeclaration>).source = null; |
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.
Out of curiosity: why is only this node not in fillOptionalPropertiesForTSESLint
?
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.
Good question. This is the situation where we have already decided to completely align AST to ts-eslint: Babel 8 generates the same AST as ts-eslint for import equals.
For other TS nodes, we can decide whether we will align to ts-eslint. If yes, we can definitely remove more cases from fillOptionalPropertiesForTSESLint
.
This PR marks the general alignment of the typescript-eslint AST. We run the alignment test against the Babel parser typescript tests and all the known AST difference is due to
tokens
orstart
/end
, which is not formally defined in the estree spec.This PR contains commits from #17233, please review from theRebased.-----BEGIN PR-----
commit placeholder.