Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add kind to TSModuleDeclaration #16732

Merged
merged 2 commits into from
Oct 23, 2024
Merged

Add kind to TSModuleDeclaration #16732

merged 2 commits into from
Oct 23, 2024

Conversation

liuxingbaoyu
Copy link
Member

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

@babel-bot
Copy link
Collaborator

babel-bot commented Aug 10, 2024 •
edited
Loading

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

@liuxingbaoyu liuxingbaoyu force-pushed the TSModuleDeclaration-kind branch 2 times, most recently from 48eccee to 5ba2c05 Compare August 10, 2024 08:37
nicolo-ribaudo
nicolo-ribaudo reviewed Aug 12, 2024
View reviewed changes
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually be curious here if ts-eslint is interested in not having this kind:

  • kind: "global" is just a repetition over global: true
  • module X {} is being deprecated and people should instead use namespace X {}, so this is similar to how for import {} assert { ... } we still generate the same AST as for import {} with { ... }

nicolo-ribaudo
nicolo-ribaudo reviewed Aug 12, 2024
View reviewed changes
packages/babel-generator/src/generators/typescript.ts

if (declare) {
this.word("declare");
this.space();
}

if (!node.global) {
this.word(id.type === "Identifier" ? "namespace" : "module");
this.word(kind ?? (id.type === "Identifier" ? "namespace" : "module"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we add the kind to the AST I would prefer to not do this change, since we currently always emit "good" code and there is no need to emit module (similarly to how we, for example, always emit semicolons even if they are absent in the input code).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the new behavior, Babel prints as is and it's fine. The deprecation is handed over to TS and ts-eslint, which will be convenient for people to use Babel to make code modifications.
In addition, the current behavior is also strange, it only outputs some module as is,

@liuxingbaoyu
Copy link
Member Author

kind: "global" is just a repetition over global: true

They deprecated global: true.

module X {} is being deprecated and people should instead use namespace X {}, so this is similar to how for import {} assert { ... } we still generate the same AST as for import {} with { ... }

Tools like Prettier need to distinguish this

@JoshuaKGoldberg
Copy link
Contributor

Back-linking for reference: typescript-eslint/typescript-eslint#6443

JoshuaKGoldberg
JoshuaKGoldberg approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@JoshuaKGoldberg JoshuaKGoldberg mentioned this pull request Aug 13, 2024
Open
9 tasks
@nicolo-ribaudo nicolo-ribaudo force-pushed the TSModuleDeclaration-kind branch from 5ba2c05 to 126ec83 Compare August 13, 2024 14:28
@nicolo-ribaudo nicolo-ribaudo added PR: Internal 🏠 A type of pull request used for our changelog categories PR: New Feature 🚀 A type of pull request used for our changelog categories and removed PR: Spec Compliance 👓 A type of pull request used for our changelog categories PR: New Feature 🚀 A type of pull request used for our changelog categories labels Aug 13, 2024
@nicolo-ribaudo
Copy link
Member

@liuxingbaoyu I wouldn't mark this as "spec compliance", as there is no spec

@nicolo-ribaudo nicolo-ribaudo added this to the v7.26.0 milestone Aug 13, 2024
@nicolo-ribaudo
Copy link
Member

module X {} is being deprecated and people should instead use namespace X {}, so this is similar to how for import {} assert { ... } we still generate the same AST as for import {} with { ... }

I wonder whether we should drop global: true in Babel 8. If typescript-eslint/parser is going to drop it in the next major and given that there is already an alternative, we may as well do it now.

@liuxingbaoyu
Copy link
Member Author

I wonder whether we should drop global: true in Babel 8. If typescript-eslint/parser is going to drop it in the next major and given that there is already an alternative, we may as well do it now.

Yes, I plan to open a PR to remove it in Babel 8!

@nicolo-ribaudo nicolo-ribaudo added the PR: Needs Review A pull request awaiting more approvals label Oct 23, 2024
@nicolo-ribaudo
Copy link
Member

Could you rebase?

@nicolo-ribaudo nicolo-ribaudo removed the PR: Needs Review A pull request awaiting more approvals label Oct 23, 2024
@liuxingbaoyu liuxingbaoyu force-pushed the TSModuleDeclaration-kind branch from 126ec83 to b4d7b6f Compare October 23, 2024 14:46
@nicolo-ribaudo nicolo-ribaudo merged commit 10f15bb into babel:main Oct 23, 2024
54 checks passed
@davidjbradshaw davidjbradshaw mentioned this pull request Oct 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

@JoshuaKGoldberg JoshuaKGoldberg JoshuaKGoldberg approved these changes

Assignees
No one assigned
Labels
area: typescript pkg: generator pkg: parser pkg: types PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Milestone
v7.26.0
Development

Successfully merging this pull request may close these issues.
4 participants
@liuxingbaoyu @babel-bot @JoshuaKGoldberg @nicolo-ribaudo