-
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
Expose printing functionality of emitter as a public API #13761
Conversation
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.
Can you explain the changes in emitter.ts a bit more? I see the reorganisation to expose the new public API, but it looks like there's two other things going on too:
- Refactor the
pipeline*
internals To make them more obviously asynchronous or something? It looks like what I would call continuation-passing style, which it didn't beforehand. - Other small improvements inside, for example,
emitSignatureAndBody
andemitTypeAssertionExpression
and the rest of the file. I think these are unrelated to the API change.
Am I totally off-base about (1) and (2)? I think more explanation would help me.
@@ -198,9 +198,9 @@ namespace ts { | |||
} | |||
|
|||
// Leading comments are emitted at /*leading comment1 */space/*leading comment*/space | |||
emitPos(commentPos); | |||
if (emitPos) emitPos(commentPos); |
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.
why this check (and other checks below) are necessary? If emitPos
can be undefined, can you please reflect this in its type?
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.
Source map support is not currently exposed publicly, so a printer created through the public API would not be able to provide an emitPos
callback. I'll amend the parameter to include undefined
.
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, I would like to know the plan for making the transforms API public as well, and how it would be used with the printer
src/compiler/types.ts
Outdated
} | ||
|
||
export interface PrintHandlers { | ||
hasGlobalName?: (name: string) => boolean; |
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.
Please add some comments on how the callbacks are used.
@sandersn both (1) and (2) are incorrect, there were no changes to that functionality of emitter, other than moving |
nice |
This exposes a new public
createPrinter
export on thets
namespace that can be used to print a TypeScript AST as-is (without transformation):To support our internal emit needs, we also have the following internal API for printing:
The public API for printing does not support source maps currently as this is heavily tied into the emitter. We may choose to support this via a later PR.
This further extends the work started in #5595 with the overall goal of making significant portions of the new transformation and emit pipeline part of the public API. This is also intended to help support ongoing effort into refactoring and code actions.
Fixes #13762