Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Incorrect elision of import {} with TS transform disabled #801

Closed
alangpierce opened this issue Jun 30, 2023 · 1 comment · Fixed by #810
Closed

Incorrect elision of import {} with TS transform disabled #801

alangpierce opened this issue Jun 30, 2023 · 1 comment · Fixed by #810

Comments

@alangpierce
Copy link
Owner

As I was working on import elision behavior, I noticed a bug when handling this code snippet:

import {} from "./a";

The expected behavior is:

  • With the TS transform enabled, the import is elided, so ./a is never evaluated at runtime. TS playground link. This happens because "all imports are type-only" is vacuously true.
  • With the TS transform disabled, the import is preserved, so ./a is evaluated at runtime. This is because import elision is a TS feature, so in plain JS there's no reason to do it.

Sucrase handles the first case correctly, but when the TS transform is disabled, it incorrectly removes the import, so this issue tracks the fix for that.

@alangpierce
Copy link
Owner Author

Another case to consider is the Flow behavior for this code. Babel and flow-remove-types have different import elision behavior in general (Babel removes an import if all named imports are removed, which flow-remove-types doesn't do), but they both preserve the import in this case, so I'll change Sucrase to do that. For flow cases like import {type a} from './a';, I'll keep Babel's behavior of removing the whole line, which is what Sucrase is already doing.

alangpierce added a commit that referenced this issue Jul 13, 2023
Fixes #801

Sucrase previously had a somewhat unified approach to import/export elision, but
this caused an issue with `import {}` statements, which should be removed in
TypeScript, but not in Flow or with neither transform enabled. Now, we plumb the
full configuration to both transforms and use the configuration to decide which
strategy is relevant.
alangpierce added a commit that referenced this issue Jul 13, 2023
Fixes #801

Sucrase previously had a somewhat unified approach to import/export elision, but
this caused an issue with `import {}` statements, which should be removed in
TypeScript, but not in Flow or with neither transform enabled. Now, we plumb the
full configuration to both transforms and use the configuration to decide which
strategy is relevant.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant