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.

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

Support TypeScript 4.5 type-only import/export specifiers #13802

Merged
merged 27 commits into from Oct 28, 2021

Conversation

sosukesuzuki
Copy link
Member

@sosukesuzuki sosukesuzuki commented Sep 30, 2021

Q                       A
Fixed Issues? Fixes #13799
Minor: New Feature? Y
Tests Added + Pass? Yes
License MIT

Support type-only import/export specifiers will be landed in TS 4.5.

import { type Foo } from "foo";
export { type Foo } from "foo";

Todo:

  • babel-parser
  • babel-generator
  • babel-types
  • babel-plugin-transform-typescript

AST change:

/cc @bradzacher What do you think?

  • Add importKind: "value" | "type" to ImportSpecifier.
  • Add exportKind: "value" | "type" to ExportSpecifier

References:

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 30, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1f933af:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@babel-bot
Copy link
Collaborator

babel-bot commented Sep 30, 2021

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

@sosukesuzuki sosukesuzuki added area: typescript pkg: generator pkg: types PR: New Feature 🚀 A type of pull request used for our changelog categories labels Sep 30, 2021
@bradzacher
Copy link
Contributor

I'd personally have preferred if the AST was just kind to match the top-level names - but considering we're matching flow here - I think it's okay for us to align with them

@sosukesuzuki sosukesuzuki marked this pull request as ready for review September 30, 2021 22:46
@fedeci fedeci added this to the v7.16.0 milestone Sep 30, 2021
@fedeci fedeci self-requested a review September 30, 2021 22:50
@@ -229,9 +229,23 @@ export default declare((api, opts) => {
continue;
Copy link
Contributor

@JLHwung JLHwung Oct 2, 2021

Choose a reason for hiding this comment

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

Can you rebase on latest main? We should also register global type for type-only import specifiers so they can be removed in the following case:

import { type A } from "module";
export { A }

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.

Can you add a test for import { type A } from "x" with onlyRemoveTypeImports? import "x" should not be removed.

@@ -40,6 +40,11 @@ export function ExportDefaultSpecifier(
}

export function ExportSpecifier(this: Printer, node: t.ExportSpecifier) {
if (node.exportKind === "type") {
this.word(node.exportKind);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.word(node.exportKind);
this.word("type");

Copy link
Member Author

Choose a reason for hiding this comment

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

d337f69 👍

} else {
allElided = false;
NEEDS_EXPLICIT_ESM.set(path.node, false);
if (!importsToRemove.includes(binding.path)) {
Copy link
Member

Choose a reason for hiding this comment

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

We can use a Set for importsToRemove, since .has() is O(1) while .includes is O(n).

Copy link
Member Author

Choose a reason for hiding this comment

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

8cb6dc0 👍

@sosukesuzuki
Copy link
Member Author

sosukesuzuki commented Oct 11, 2021

@nicolo-ribaudo Thank you for your review! I've addressed your comments. 4d1cde6

@@ -288,7 +288,7 @@ export default declare((api, opts) => {
}
}

if (isAllSpecifiersElided()) {
if (!onlyRemoveTypeImports && isAllSpecifiersElided()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why should we keep import statements when onlyRemoveTypeImports is true?

TS removes them anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo You said #13802 (review). What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Whoops, I assumed that import { type X } from "x" would have been equivalent to import {} from "x" and thus not removed. I'm sorry!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, interesting. TS removes import {} from "x", too. It preserves only import "x".

Copy link
Member Author

Choose a reason for hiding this comment

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

According to behavior of TypeScript, it is better to remove the Import Statement in such cases. 6b79fd1

const local = this.parseModuleExportName();
node.local = local;
if (this.eatContextual(tt._as)) {
node.local = this.parseModuleExportName();
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to avoid mixing TS/flow-specific code in the main parser, when possible.

Would it be possible to move type handling in parseModuleExportName()? Or maybe we reorganize it like this:

parseExportSpecifiers() {
  while () { // loop to parse all the specifiers
    const local = this.parseModuleExportName();
    nodes.push(this.parseExportSpecifier(local));
  }
}

parseExportSpecifier(local) {
  // ...
}

and in typescript/index.js

parseExportSpecifier(local) {
  if (local.type === "Identifier" && local.name === "type") {
    const node = this.startNodeAtNode(local);
    // if this is a type export, parse it as such
    return node;
  }
  return super.parseExprtSpecifier(local);
}

This is similar to what we do for imports.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a little different from the interface you suggested, but I modified it based on that. Please review again.

commits:

(Sorry for many commits..)

node[rightOfAsKey] = rightOfAs;

const kindKey = isImport ? "importKind" : "exportKind";
node[kindKey] = hasTypeSpecifier ? "type" : "value";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also update packages/babel-parser/src/types.js?

Copy link
Member Author

Choose a reason for hiding this comment

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

c44171a 👍

// { type as as something }
hasTypeSpecifier = true;
leftOfAs = firstAs;
rightOfAs = this.parseIdentifier();
Copy link
Contributor

Choose a reason for hiding this comment

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

When we are parsing export, the right-of-as is a liberal identifier (should be parsed by parseIdentifier(true)), otherwise it should throw unexpected reserved word:

export { type a as if } from "x"; // valid
import { type a as if } from "x"; // invalid

Copy link
Member Author

Choose a reason for hiding this comment

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

We can use parseIdentifier(true/false) for those specifiers. But Babel doesn't check reserved word for TypeScript. Should we still fix for that?

checkReservedWord(
word: string, // eslint-disable-line no-unused-vars
startLoc: number, // eslint-disable-line no-unused-vars
checkKeywords: boolean, // eslint-disable-line no-unused-vars
// eslint-disable-next-line no-unused-vars
isBinding: boolean,
): void {
// Don't bother checking for TypeScript code.
// Strict mode words may be allowed as in `declare namespace N { const static: number; }`.
// And we have a type checker anyway, so don't bother having the parser do it.
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, let's fix that in another PR, then.

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Awesome work!

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.

Thanks!

@nicolo-ribaudo nicolo-ribaudo merged commit d5ba355 into babel:main Oct 28, 2021
@sosukesuzuki sosukesuzuki deleted the type-only-import-specifier branch October 29, 2021 01:58
@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 Jan 28, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: generator pkg: parser pkg: types PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TS 4.5] Support type-only import/export specifies
6 participants