Skip to content

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Apr 11, 2025

Q                       A
Fixed Issues? Closes #16679
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

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 or start/end, which is not formally defined in the estree spec.

This PR contains commits from #17233, please review from the -----BEGIN PR----- commit placeholder. Rebased.

@babel-bot
Copy link
Collaborator

babel-bot commented Apr 11, 2025

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/59091

@JLHwung JLHwung force-pushed the add-ts-eslint-integration-js-test-part-3 branch from fd2f90d to 09de007 Compare April 12, 2025 13:04
return;
case "TSModuleDeclaration":
node.declare ??= false;
node.global ??= node.kind === "global";
Copy link
Member

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?

Copy link
Contributor Author

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.

Comment on lines +3029 to +3032
(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;
Copy link
Member

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?

Copy link
Contributor Author

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.

@JLHwung JLHwung merged commit 3766c4d into babel:main Apr 14, 2025
55 checks passed
@JLHwung JLHwung deleted the add-ts-eslint-integration-js-test-part-3 branch April 14, 2025 00:27
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jul 14, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[babel 8] Align AST with typescript-eslint
4 participants