Skip to content

Fix namespace import/export helper usage under '--esModuleInterop' #39490

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

Merged
merged 3 commits into from
Jul 13, 2020

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Jul 8, 2020

This fixes bugs in both check and emit for import * as ns ... and export * as ns ... and import { default as ... }, export { default } ... and export { default as ... } when esModuleInterop is true:

  • We failed to use tslib for export * as ns under --importHelpers --esModuleInterop --target commonjs.
  • We were incorrectly checking for the existence of the __exportStar helper for export * as ns, which actually uses the __importStar helper under --esModuleInterop (and no helper at all when not in that mode).
  • We were not checking for the existence __importStar helper for import * as ns when under --esModuleInterop.
  • We were not checking for the existence of __importDefault helper for import { default as ..., export { default as ... or export { default } ... when under --esModuleInterop.
  • We were not emitting __importStar for export * as ns under --esModuleInterop --target amd, though we do for import * as ns from ...; export { ns }; and for both export * as ns and import * as ns in commonjs.
  • We were not emitting __importStar for export { default as ... or export { default } ... under --esModuleInterop.

Fixes #37113
Fixes #35800

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

We should also make sure #34903 is on our radar, since the fix for that'll touch some of the same stuff in the same ways (so we should probably try to fix near the same time), since the bug is similar but with export {a,b} from "ns" instead of export * as a from "ns".

@rbuckton
Copy link
Member Author

rbuckton commented Jul 8, 2020

I'm looking into rolling a fix for #35800 into this as well.

@rbuckton
Copy link
Member Author

rbuckton commented Jul 9, 2020

@weswigham this was actually originally intended to address issues with export * as ns introduced by #34903 (see #37113). Is there something more I should be aware of?

@rbuckton
Copy link
Member Author

rbuckton commented Jul 9, 2020

@weswigham can you take one more look, since I added in the fix for export { default } from and export { default as ns } from since it needed the same kind of coverage?

# Conflicts:
#	src/compiler/transformers/module/module.ts
@rbuckton
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 14, 2020

Heya @rbuckton, I've started to run the tarball bundle task on this PR at 5ad8532. You can monitor the build here.

@rbuckton rbuckton restored the fix37113 branch July 14, 2020 00:42
@rbuckton
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 14, 2020

Heya @rbuckton, I've started to run the tarball bundle task on this PR at 5ad8532. You can monitor the build here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants