-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Elide re-exports of unresolved type-only imports #56449
Conversation
@@ -47581,7 +47581,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
} | |||
const target = getExportSymbolOfValueSymbolIfExported(resolveAlias(symbol)); | |||
if (target === unknownSymbol) { | |||
return true; | |||
return !excludeTypeOnlyValues || !getTypeOnlyAliasDeclaration(symbol); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need a test for the other condition? I think the test covers the latter only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!excludeTypeOnlyValues
is never covered under any test because I’m pretty sure it’s never correct to pass false
for that. I fixed one case that forgot to pass true
for it in #55097, and the last remaining caller didn’t change any baselines when I had it pass true
. I made a note to look at it and probably remove the parameter altogether in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, the last place that doesn’t pass true
is checking whether to elide an ImportEqualsDeclaration, which doesn’t strictly matter because it’s not a runtime error to require()
a module that doesn’t export anything. But it should probably be elided anyway, so it’s likely that parameter will go away and we’ll treat it as if it’s always true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, thanks!
@typescript-bot cherry-pick this to release-5.3 |
Heya @andrewbranch, I've started to run the task to cherry-pick this into |
Hey @andrewbranch, I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release-5.3 manually. |
Fixes #56445