-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Error on export default of type: issue https://github.com/microsoft/TypeScript/issues/55087 #55097
Error on export default of type: issue https://github.com/microsoft/TypeScript/issues/55087 #55097
Conversation
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
@microsoft-github-policy-service agree |
Fixes: #55087
|
FWIW I think it's okay to default-export types in general, as long as they only have a typespace meaning--the problem is when you default-export something that has a valuespace meaning (like a class) which has been imported via |
If this is the only condition - I think you can just convert if (!isIllegalExportDefaultInCJS && !(node.flags & NodeFlags.Ambient) && compilerOptions.verbatimModuleSyntax && getTypeOnlyAliasDeclaration(sym, SymbolFlags.Value)) { to if (!(node.flags & NodeFlags.Ambient) && getTypeOnlyAliasDeclaration(sym, SymbolFlags.Value)) { the reason I find this a little awkward is checking with ts-fork-typechecker but emitting with ts loader transpileOnly is pretty common and you can pass the typechecking stage and still emit invalid code as the following code: ts.transpileModule(`
import {type Node} from "typescript"
export default Node
`, {}) will give you export default Node even though Node is an interface. I can understand why you (the TS team) would say this is improper use and shouldn't be supported though. With that said, my take is:
But I will implement whatever you think is best The best solution IMO is to not change compile time behavior at all and for emit to just work but based on your comments in the issue I am unsure if this is possible. I know less than little about the emit phase so I don't have any idea here. |
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.
I think the expected behavior is
- an
export default
of a type-only import never emits anything - it’s an error only in
isolatedModule
(ts1205)
Fixed the checker behavior, still need to fix the emit. (Disregard the review request, I jumped the gun.) |
Ok, ready. |
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.
LGTM but should get the extended test run.
@@ -41,7 +41,6 @@ exports.A = A; | |||
//// [b.js] | |||
"use strict"; | |||
Object.defineProperty(exports, "__esModule", { value: true }); | |||
exports.default = types; |
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.
Oops!
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.
Yeppp
@typescript-bot test top200 @typescript-bot perf test this |
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 12f9ae6. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 12f9ae6. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the regular perf test suite on this PR at 12f9ae6. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 12f9ae6. You can monitor the build here. Update: The results are in! |
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Hey @jakebailey, the results of running the DT tests are ready. |
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
These errors from the top repos look possibly wrong, especially the playwright one. Need to take a closer look. |
@typescript-bot test top200 |
Heya @andrewbranch, I've started to run the diff-based top-repos suite on this PR at 7b2dee6. You can monitor the build here. Update: The results are in! |
You can ignore that playwright one, that is currently happening on every PR and we just haven't looked at why. |
Huh? The playwright one is clearly caused by this and fixed in the latest commit. |
Apologies, I mixed up the "P" libraries and confused playwright for puppeteer (which you can see on other PRs like #56429 (comment)). Ignore me! |
@andrewbranch Here are the results of running the top-repos suite comparing Everything looks good! |
Fixes: #55087
This changes the behavior of TS to the specified behavior in issue.
Export default of types become errors - i.e
This is because if it does not error - typescript emits invalid code.