[ts] Insert `export {}` when necessary to imply ESM #13314

wbinnssmith · 2021-05-14T23:22:14Z

This diff is best viewed with whitespace diff disabled: https://github.com/babel/babel/pull/13314/files?w=1

This makes the TypeScript transformer insert an empty export {} declaration into files that would otherwise start as modules but become ambiguous after transformation, specifically when there are no imports of exports in the transformed result. See #12568 for context.

Since type imports and exports are removed, this change tracks when imports and exports are left behind as value imports/exports. If there are any left behind, the export {} expression does not get added.

Q	A
Fixed Issues?	Fixes #12568
Patch: Bug Fix?	No
Major: Breaking Change?	No
Minor: New Feature?	Yes
Tests Added + Pass?	Adjusted existing, which cover use
Documentation PR Link	None
Any Dependency Changes?	None
License	MIT

codesandbox-ci · 2021-05-14T23:25:39Z

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 0fe318f:

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

wbinnssmith · 2021-05-14T23:22:50Z

packages/babel-plugin-transform-typescript/src/index.ts

@@ -63,6 +63,8 @@ export default declare((api, opts) => {
    var { allowDeclareFields = false } = opts;
  }

+  let hasValueExports = false;


I believe this could also be attached to state instead of closing over it

We can either use a WeakMap from the program node to this variable, or attach it to state. I prefer the WeakMap approach since it's similar to what we use in many other plugins.

Closing over causes problems, because Babel doesn't reinstantiate the plugin for each file but only when the options change.

Btw, do we need both hasValueExports/hasValueImports, or can it be a single boolean?

I prefer the WeakMap approach since it's similar to what we use in many other plugins.

Great idea 🙂

Btw, do we need both hasValueExports/hasValueImports, or can it be a single boolean?

Nope, only one is necessary. I'll combine them.

JLHwung

LGTM with some nits.

For reviewers: please ignore whitespace changes when reviewing.

JLHwung · 2021-05-18T02:14:30Z

packages/babel-plugin-transform-typescript/src/index.ts

        }
+
+        IMPLICITLY_ESM.add(state.file.ast.program);


nit: Alternatively we can add this to the ExportDeclaration visitor, it covers ExportExportAllDeclaration, ExportDefaultDeclaration and ExportNamedDeclaration.

We would have to be careful about ordering, since we only want to add the node to the WeakSet if it's not removed.

JLHwung · 2021-05-18T02:14:55Z

packages/babel-plugin-transform-typescript/src/index.ts

        }
+
+        IMPLICITLY_ESM.add(state.file.ast.program);


Q: Why do we need this for ExportSpecifier?

Yeah good point — looks like this is already handled above.

nicolo-ribaudo

https://github.com/babel/babel/pull/13314/files?diff=unified&w=1#r633982388 could probably be removed, but except for that lgtm 👍

nicolo-ribaudo · 2021-05-18T15:43:01Z

Oh it looks like GH actions are down

Regarding the failing test, I checked how TS handles var a: string: it doesn't inject export {} (https://www.typescriptlang.org/play?module=6#code/G4QwTgBCBcEM4BcwEsB2BzA3EA). However, with this PR Babel transforms it to var a; export {};. I think it would be better to align our behavior and only inject export {} if we actually removed import/export statements.

babel-bot · 2021-05-18T21:24:24Z

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

nicolo-ribaudo · 2021-05-19T14:14:21Z

Could you address #13314 (comment)? 🙏

wbinnssmith · 2021-05-20T02:08:58Z

Could you address #13314 (comment)? 🙏

Ah, missed what you meant here. Just implemented this and checked a few of Babel's test cases agains the typescript playground and they behave the same now 😄

packages/babel-plugin-transform-typescript/src/index.ts

nicolo-ribaudo · 2021-05-20T22:54:50Z

Thanks!

wbinnssmith changed the title ~~Export an empty named export declaration to imply ESM~~ TypeScript transform: export an empty named export declaration to imply ESM May 14, 2021

wbinnssmith changed the title ~~TypeScript transform: export an empty named export declaration to imply ESM~~ TypeScript transform: insert an empty named export declaration when necessary to imply ESM May 14, 2021

wbinnssmith commented May 14, 2021

View reviewed changes

fedeci added area: typescript PR: Spec Compliance 👓 A type of pull request used for our changelog categories labels May 15, 2021

wbinnssmith force-pushed the wbinnssmith/empty-export-named-decl branch from 23a2480 to 8589908 Compare May 18, 2021 00:18

JLHwung approved these changes May 18, 2021

View reviewed changes

nicolo-ribaudo previously approved these changes May 18, 2021

View reviewed changes

wbinnssmith force-pushed the wbinnssmith/empty-export-named-decl branch 2 times, most recently from 8bd591c to ba831ff Compare May 18, 2021 21:20

existentialism approved these changes May 19, 2021

View reviewed changes

wbinnssmith force-pushed the wbinnssmith/empty-export-named-decl branch from ba831ff to 97f9a7e Compare May 20, 2021 02:06

nicolo-ribaudo approved these changes May 20, 2021

View reviewed changes

nicolo-ribaudo reviewed May 20, 2021

View reviewed changes

packages/babel-plugin-transform-typescript/src/index.ts Outdated Show resolved Hide resolved

Export an empty named export declaration to imply ESM

0fe318f

wbinnssmith force-pushed the wbinnssmith/empty-export-named-decl branch from 97f9a7e to 0fe318f Compare May 20, 2021 17:45

JLHwung approved these changes May 20, 2021

View reviewed changes

lweathermon approved these changes May 20, 2021

View reviewed changes

nicolo-ribaudo changed the title ~~TypeScript transform: insert an empty named export declaration when necessary to imply ESM~~ [ts] Insert export {} when necessary to imply ESM May 20, 2021

nicolo-ribaudo merged commit b4c798e into babel:main May 20, 2021

sync-by-unito bot mentioned this pull request May 31, 2021

chore(deps-dev): bump @babel/eslint-parser from 7.13.8 to 7.14.4 filecoin-project/slate#772

Closed

SimenB mentioned this pull request Jun 4, 2021

chore: bump deps jestjs/jest#11523

Merged

vsumner mentioned this pull request Jun 4, 2021

Babel plugin transform typescript 7.14.4 Shopify/web-configs#256

Merged

4 tasks

This was referenced Jun 10, 2021

chore(deps-dev): bump @babel/eslint-parser from 7.13.14 to 7.14.5 filecoin-project/slate#779

Closed

chore(deps-dev): bump @babel/core from 7.13.14 to 7.14.5 filecoin-project/slate#786

Closed

o0Djeen0o mentioned this pull request Jun 16, 2021

Compilation error with recent version of Babel parser ui5-community/babel-plugin-transform-modules-ui5#52

Closed

rugirundao mentioned this pull request Jun 23, 2021

https://github.com/babel/babel/pull/13314 facebook/create-react-app#11140

Closed

devongovett mentioned this pull request Jun 26, 2021

Runtime error when re-exporting empty module parcel-bundler/parcel#6361

Closed

GuillaumedesPommareSAP mentioned this pull request Jul 1, 2021

Fix ExportNamedDeclaration with babel 7.14.4 ui5-community/babel-plugin-transform-modules-ui5#53

Merged

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Aug 20, 2021

github-actions bot locked as resolved and limited conversation to collaborators Aug 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ts] Insert `export {}` when necessary to imply ESM #13314

[ts] Insert `export {}` when necessary to imply ESM #13314

wbinnssmith commented May 14, 2021 •

edited

codesandbox-ci bot commented May 14, 2021 •

edited

wbinnssmith May 14, 2021

nicolo-ribaudo May 15, 2021 •

edited

wbinnssmith May 17, 2021

JLHwung left a comment

JLHwung May 18, 2021

nicolo-ribaudo May 18, 2021

JLHwung May 18, 2021

wbinnssmith May 18, 2021

nicolo-ribaudo left a comment

nicolo-ribaudo commented May 18, 2021

babel-bot commented May 18, 2021 •

edited

nicolo-ribaudo commented May 19, 2021

wbinnssmith commented May 20, 2021 •

edited

nicolo-ribaudo commented May 20, 2021

[ts] Insert export {} when necessary to imply ESM #13314

[ts] Insert export {} when necessary to imply ESM #13314

Conversation

wbinnssmith commented May 14, 2021 • edited

codesandbox-ci bot commented May 14, 2021 • edited

wbinnssmith May 14, 2021

Choose a reason for hiding this comment

nicolo-ribaudo May 15, 2021 • edited

Choose a reason for hiding this comment

wbinnssmith May 17, 2021

Choose a reason for hiding this comment

JLHwung left a comment

Choose a reason for hiding this comment

JLHwung May 18, 2021

Choose a reason for hiding this comment

nicolo-ribaudo May 18, 2021

Choose a reason for hiding this comment

JLHwung May 18, 2021

Choose a reason for hiding this comment

wbinnssmith May 18, 2021

Choose a reason for hiding this comment

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

nicolo-ribaudo commented May 18, 2021

babel-bot commented May 18, 2021 • edited

nicolo-ribaudo commented May 19, 2021

wbinnssmith commented May 20, 2021 • edited

nicolo-ribaudo commented May 20, 2021

[ts] Insert `export {}` when necessary to imply ESM #13314

[ts] Insert `export {}` when necessary to imply ESM #13314

wbinnssmith commented May 14, 2021 •

edited

codesandbox-ci bot commented May 14, 2021 •

edited

nicolo-ribaudo May 15, 2021 •

edited

babel-bot commented May 18, 2021 •

edited

wbinnssmith commented May 20, 2021 •

edited