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
Copy link

@codesandbox codesandbox 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 babel-bot commented Sep 30, 2021

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

@bradzacher
Copy link
Contributor

@bradzacher bradzacher commented Sep 30, 2021

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 force-pushed the type-only-import-specifier branch from 3cc6c3d to 15392c3 Sep 30, 2021
@sosukesuzuki sosukesuzuki marked this pull request as ready for review Sep 30, 2021
@fedeci fedeci added this to the v7.16.0 milestone Sep 30, 2021
@fedeci fedeci self-requested a review Sep 30, 2021
@@ -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

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

@nicolo-ribaudo nicolo-ribaudo Oct 10, 2021

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

@sosukesuzuki sosukesuzuki Oct 11, 2021

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

@nicolo-ribaudo nicolo-ribaudo Oct 10, 2021

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

@sosukesuzuki sosukesuzuki Oct 11, 2021

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 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

@JLHwung JLHwung Oct 12, 2021

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

@sosukesuzuki sosukesuzuki Oct 15, 2021

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

@nicolo-ribaudo nicolo-ribaudo Oct 15, 2021

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

@JLHwung JLHwung Oct 15, 2021

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

@sosukesuzuki sosukesuzuki Oct 16, 2021

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

@nicolo-ribaudo nicolo-ribaudo Oct 24, 2021

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

@sosukesuzuki sosukesuzuki Oct 28, 2021

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

@JLHwung JLHwung Oct 25, 2021

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

@sosukesuzuki sosukesuzuki Oct 28, 2021

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

@JLHwung JLHwung Oct 25, 2021

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

@sosukesuzuki sosukesuzuki Oct 28, 2021

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

@JLHwung JLHwung Oct 28, 2021

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

Awesome work!

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Thanks!

@nicolo-ribaudo nicolo-ribaudo merged commit d5ba355 into babel:main Oct 28, 2021
27 of 29 checks passed
@sosukesuzuki sosukesuzuki deleted the type-only-import-specifier branch Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants