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

Fix invalid output code when eliding TS star imports #723

Merged
merged 1 commit into from Jul 14, 2022

Conversation

alangpierce
Copy link
Owner

From some manual testing, I noticed that the code:

import A, * as B from "./A";
console.log(A);

becomes this invalid code:

import A, from "./A";
console.log(A);

The * as B was being elided because it's treated as a type-only import, but in
this case we need to elide the comma as well since there's no concept of
"trailing commas" here. The code to do that is a little clunky, but I ended up
always removing the comma and using a boolean to track whether it should be
added back.

From some manual testing, I noticed that the code:
```ts
import A, * as B from "./A";
console.log(A);
```
becomes this invalid code:
```ts
import A, from "./A";
console.log(A);
```
The `* as B` was being elided because it's treated as a type-only import, but in
this case we need to elide the comma as well since there's no concept of
"trailing commas" here. The code to do that is a little clunky, but I ended up
always removing the comma and using a boolean to track whether it should be
added back.
@codecov
Copy link

codecov bot commented Jul 14, 2022

Codecov Report

Merging #723 (98ee525) into main (66da4d1) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #723      +/-   ##
==========================================
+ Coverage   87.05%   87.15%   +0.10%     
==========================================
  Files          54       54              
  Lines        5702     5708       +6     
  Branches     1343     1345       +2     
==========================================
+ Hits         4964     4975      +11     
+ Misses        470      466       -4     
+ Partials      268      267       -1     
Impacted Files Coverage Δ
src/transformers/ESMImportTransformer.ts 93.87% <100.00%> (+3.80%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@github-actions
Copy link

Benchmark results

Before this PR: 345.1 thousand lines per second
After this PR: 345.2 thousand lines per second

Measured change: 0.04% faster (0.74% slower to 0.21% faster)
Summary: Likely no significant difference

@alangpierce alangpierce merged commit 348e7aa into main Jul 14, 2022
@alangpierce alangpierce deleted the fix-ts-star-import-elision branch July 14, 2022 19:56
1Lighty pushed a commit to Astra-mod/sucrase that referenced this pull request Aug 14, 2022
From some manual testing, I noticed that the code:
```ts
import A, * as B from "./A";
console.log(A);
```
becomes this invalid code:
```ts
import A, from "./A";
console.log(A);
```
The `* as B` was being elided because it's treated as a type-only import, but in
this case we need to elide the comma as well since there's no concept of
"trailing commas" here. The code to do that is a little clunky, but I ended up
always removing the comma and using a boolean to track whether it should be
added back.
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 this pull request may close these issues.

None yet

1 participant