[ts] Insert export {} when necessary to imply ESM#13314
[ts] Insert export {} when necessary to imply ESM#13314nicolo-ribaudo merged 1 commit intobabel:mainfrom
export {} when necessary to imply ESM#13314Conversation
|
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:
|
| var { allowDeclareFields = false } = opts; | ||
| } | ||
|
|
||
| let hasValueExports = false; |
There was a problem hiding this comment.
I believe this could also be attached to state instead of closing over it
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
23a2480 to
8589908
Compare
JLHwung
left a comment
There was a problem hiding this comment.
LGTM with some nits.
For reviewers: please ignore whitespace changes when reviewing.
| return; | ||
| } | ||
|
|
||
| IMPLICITLY_ESM.add(state.file.ast.program); |
There was a problem hiding this comment.
nit: Alternatively we can add this to the ExportDeclaration visitor, it covers ExportExportAllDeclaration, ExportDefaultDeclaration and ExportNamedDeclaration.
There was a problem hiding this comment.
We would have to be careful about ordering, since we only want to add the node to the WeakSet if it's not removed.
| return; | ||
| } | ||
|
|
||
| IMPLICITLY_ESM.add(state.file.ast.program); |
There was a problem hiding this comment.
Q: Why do we need this for ExportSpecifier?
There was a problem hiding this comment.
Yeah good point — looks like this is already handled above.
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
https://github.com/babel/babel/pull/13314/files?diff=unified&w=1#r633982388 could probably be removed, but except for that lgtm 👍
|
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.
8bd591c to
ba831ff
Compare
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/46307/ |
|
Could you address #13314 (comment)? 🙏 |
ba831ff to
97f9a7e
Compare
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 😄 |
97f9a7e to
0fe318f
Compare
export {} when necessary to imply ESM
|
Thanks! |
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.