-
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
Support export type *
#52217
Support export type *
#52217
Conversation
@typescript-bot perf test this |
Heya @andrewbranch, I've started to run the diff-based top-repos suite on this PR at 08701ac. You can monitor the build here. Update: The results are in! |
Heya @andrewbranch, I've started to run the perf test suite on this PR at 08701ac. You can monitor the build here. Update: The results are in! |
4a8519f
to
3b6a185
Compare
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 can't say I saw anything surprising in the checker (looked as I would guess), but I think the baselines say it all and look good to me; all of the edge cases I was coming up with appear to be tested.
@andrewbranch Here they are:
CompilerComparison Report - main..52217
System
Hosts
Scenarios
TSServerComparison Report - main..52217
System
Hosts
Scenarios
StartupComparison Report - main..52217
System
Hosts
Scenarios
Developer Information: |
@andrewbranch Here are the results of running the top-repos suite comparing Everything looks good! |
@typescript-bot pack this |
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 3b6a185. You can monitor the build here. |
Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
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.
Unless I'm mistaken, it looks like this needs some (error-free) .d.ts
emit tests to ensure we preserve the export type
y-ness of these declarations when doing declaration emit (and probably some emitter/declaration emitter code to match). A test with an export type
declaration in a js file with js declaration emit on would also be relevant, despite (I assume) being an error.
Good call. I suspect it already works since this form is not actually new syntax, just newly not a grammar error, but it definitely needs to be tested. |
As I predicted, declaration emit does work. It did drop the |
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.
It'd be nice if the js declaration emit preserved the type
modifier, just in case someone uses the (admittedly internal) node builder APIs on typescript code (which is why it knows how to emit interfaces and such), but since it's an error it's non-critical.
On second thought, having consistent declaration emit is also required to make errors consistent between referenced projects in the editor and on the CLI, so I went ahead and fixed it. |
Fixes #37238
Adds support for
export type * from "mod"
andexport type * as ns from "mod"
.