Elide type-only exports when the TS transform is enabled #433
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes #398 This ended up having a *lot* of cases and complexity. We actually care about the known type declarations and the known value declarations. * In ESM mode, only elide exports that are known to be a type and not known to be a value. This is true both for named export syntax and default export. * In CJS mode, elide named exports that are either completely unknown or known to be a type. For default exports, it's the same logic as ESM: we only elide if we know it as a type and not as a value. This required going through all declarations (at least in TS parsing mode) and making sure we add the right identifier info to each of them, which in turn had consequences on the react-hot-loader transform, so that had to be fixed. One challenge is that default exports should be elided only when they export a single identifier, which we can't know statically. Instead, we now use the `rhsEndIndex` field on the `export` token to know when the export statement actually ends, and use that to determine if the export is just an identifier.
Codecov Report
@@ Coverage Diff @@
## master #433 +/- ##
==========================================
+ Coverage 80.22% 80.43% +0.21%
==========================================
Files 47 49 +2
Lines 5041 5122 +81
Branches 1175 1199 +24
==========================================
+ Hits 4044 4120 +76
- Misses 696 700 +4
- Partials 301 302 +1
Continue to review full report at Codecov.
|
alangpierce
added a commit
that referenced
this pull request
Dec 1, 2020
Fixes #563 Fixes #473 The type-only export elision implemented in #433 had a false positive for destructure delcarations, since `getDeclarationInfo` only looks at top-level declarations and top-level expressions like `const {x} = ...` weren't being marked correctly. To fix, we can mark object shorthand declarations as top-level when applicable. I also refactored both relevant code paths to avoid the repeated assignment left-hand side.
alangpierce
added a commit
that referenced
this pull request
Dec 2, 2020
Fixes #563 Fixes #473 The type-only export elision implemented in #433 had a false positive for destructure delcarations, since `getDeclarationInfo` only looks at top-level declarations and top-level expressions like `const {x} = ...` weren't being marked correctly. To fix, we can mark object shorthand declarations as top-level when applicable. I also refactored both relevant code paths to avoid the repeated assignment left-hand side.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #398
This ended up having a lot of cases and complexity. We actually care about the
known type declarations and the known value declarations.
be a value. This is true both for named export syntax and default export.
to be a type. For default exports, it's the same logic as ESM: we only elide
if we know it as a type and not as a value.
This required going through all declarations (at least in TS parsing mode) and
making sure we add the right identifier info to each of them, which in turn had
consequences on the react-hot-loader transform, so that had to be fixed.
One challenge is that default exports should be elided only when they export a
single identifier, which we can't know statically. Instead, we now use the
rhsEndIndexfield on theexporttoken to know when the export statementactually ends, and use that to determine if the export is just an identifier.