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 for type only imports & exports #520

Closed
chrstntdd opened this issue Apr 5, 2020 · 2 comments · Fixed by #532
Closed

Support for type only imports & exports #520

chrstntdd opened this issue Apr 5, 2020 · 2 comments · Fixed by #532

Comments

@chrstntdd
Copy link

Hello there! This project is fantastic and I use it heavily. Thank you for all your hard work!

I have a project using the latest stable TypeScript version (3.8.3) and tried to use Type-Only Imports and Export but was unable to compile with sucrase - throwing the following error.

SyntaxError: Error transforming src/index.ts: Unexpected token (8:13)
// src/index.ts
// ...
import type { TerserOpts } from "./types"
// ...

Are there any plans to support this feature in sucrase? If so, is there any way I can help contribute? Would love to have sucrase support this.
Cheers!

@alangpierce
Copy link
Owner

Hi @chrstntdd , thanks for the kind words. 😄

Yep, I've been following along on type-only imports, and certainly I want Sucrase to support them. I was hoping that they'd eventually get rid of the need to detect imported identifiers as type-only or not (which causes quite a bit of complexity and slowness in Sucrase), but looks like statements like import {a, b} from 'foo'; still need to have a or b removed based on usage in the file.

From a code standpoint, getting this working will involve these steps:

  1. To implement parsing, port all recent babel-parser changes, similar to Port babel-parser changes from 2019-09-06 to 2019-12-22 #487 . That should naturally pick up parsing the type token.
  2. Change CJSImportProcessor and ESMImportProcessor to remove import type lines. I think this actually is already working because flow has the same syntax, so this just needs some TS tests I think.
  3. Maybe change CJSImportProcessor and ESMImportProcessor to optionally no longer remove type-only import lines, equivalent to importsNotUsedAsValues=preserve in TS and onlyRemoveTypeImports in Babel.

I'm actually unsure how to approach that last point, or whether to do anything at all, and I'd love feedback. I think my plan for my own projects is to use importsNotUsedAsValues=error everywhere so that I need to be explicit, and that means that the Sucrase behavior doesn't matter because I never intend to have type-only imports without type. Do you think you'd want a Sucrase flag to mimic importsNotUsedAsValues=preserve (keeping type-only import statements as side-effect-only imports)? Certainly if TS makes that the default, Sucrase will follow suit, but in general I'd prefer to keep Sucrase configuration minimal.

I think it probably makes sense for me to do step 1 since it's a bit hard to do without a lot of project context. But it would be great to get your feedback and if you could help test the feature when it's ready. At the moment, I don't have any codebases using that syntax that I can test with, so I worry that I may miss some detail.

alangpierce added a commit that referenced this issue May 11, 2020
Fixes #520

The PR #523 already added parsing support for type-only import and export
syntax, and there was already support for removing `import type` from Flow, so
the only new logic was to fully support `export type`. The implementation here
is a little ugly; Flow's version sets the tokens as type tokens, whereas the TS
version sets the types as regular tokens and removes them at transform type.
This seems to be necessary to follow the behavior that `export type` statements
do not result in the imported value being elided.

Ideally, Sucrase would support the `importsNotUsedAsValues` TS preference in
some way, but for now, it keeps the old behavior of eliding type-only imports.
The setting `importsNotUsedAsValues=error` should make the distinction
irrelevant anyway, so it seems low priority to support.
alangpierce added a commit that referenced this issue May 11, 2020
Fixes #520

The PR #523 already added parsing support for type-only import and export
syntax, and there was already support for removing `import type` from Flow, so
the only new logic was to fully support `export type`. The implementation here
is a little ugly; Flow's version sets the tokens as type tokens, whereas the TS
version sets the types as regular tokens and removes them at transform type.
This seems to be necessary to follow the behavior that `export type` statements
do not result in the imported value being elided.

Ideally, Sucrase would support the `importsNotUsedAsValues` TS preference in
some way, but for now, it keeps the old behavior of eliding type-only imports.
The setting `importsNotUsedAsValues=error` should make the distinction
irrelevant anyway, so it seems low priority to support.
@alangpierce
Copy link
Owner

Sorry for the delay! I just released this as Sucrase 3.14.0. Let me know if you see any issues!

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

Successfully merging a pull request may close this issue.

2 participants