Skip to content

Syntactically important comments removed from emitted code (@deprecated and others) #51177

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

Closed
WillsterJohnson opened this issue Oct 14, 2022 · 5 comments
Labels
Out of Scope This idea sits outside of the TypeScript language design constraints Suggestion An idea for TypeScript

Comments

@WillsterJohnson
Copy link

WillsterJohnson commented Oct 14, 2022

Bug Report

🔎 Search Terms

remove comments, declaration, syntax, deprecated, jsdoc

🕗 Version & Regression Information

  • This is the behavior in every version I tried

⏯ Playground Link

Playground link (Observe d.ts output)

💻 Code

/**
 * @deprecated
 */
 function foo() {
   return "bar";
 }

🙁 Actual behavior

The JSDoc comment is removed from the d.ts file

🙂 Expected behavior

The JSDoc comment contains syntax which is recognised by most editors and developers to have significant meaning regarding the associated object. It is part of the language and should not be removed from declaration files.

removeComments is documented here with the following text;

Additional Info

Strips all comments from TypeScript files when converting into JavaScript. Defaults to false.

/** The translation of 'Hello world' into Portuguese */
export const helloWorldPTBR = "Olá Mundo";

When removeComments is set to true:

export const helloWorldPTBR = "Olá Mundo";

Without setting removeComments or having it as false:

/** The translation of 'Hello world' into Portuguese */
export const helloWorldPTBR = "Olá Mundo";

This means that your comments will show up in the JavaScript code.

I would like to emphasise; "when converting into JavaScript"

I would also like to point out; d.ts files are NOT JavaScript.

The documentation does not state anywhere that JSDoc will be removed from declaration files.

You're probably thinking...

"turn off removeComments"

I shouldn't have to effect the size of my JS output for my dts output to be syntactically correct.

"use a 3rd party minifier"

I shouldn't have to use 3rd party tools for TypeScript to have syntactically correct output.

"this is related the remove comments issues"

Yeah, there are a few of those.

(#14619) (#45187) (#26987) (#30307) (#20441)

What they fail to identify is that TypeScript fails to output syntactically correct dts when compiling with removeComments.

function foo(): string;

and

/**
 * @deprecated
 */
function foo(): string;

have completely different meanings. The former being used in place of the latter is as bad as object in place of MyInterface; significant information is destroyed by the compiler.

This is a bug which leaves every user to choose between;

a) broken dist dts

b) over-sized dist js

c) use a 3rd party library / hacky build process to fix the bug

@RyanCavanaugh
Copy link
Member

"this is related to (thing)! close this issue!"

Maintainer snipes are not constructive or helpful, whether here or in the other thread. Please stop.

What's the difference between this issue and #14619? It sounds like you're asking for an in-between option between "no comments" and "all comments" that is "(Some? All?) JS doc comments" ?

@fatcerberus
Copy link

fatcerberus commented Oct 16, 2022

Implied feature request seems to be “preserve JSDoc in generated .d.ts even with removeComments enabled”

@WillsterJohnson
Copy link
Author

@RyanCavanaugh as mentioned, when specifying removeComments TypeScript removes content which has meaning relative to the code, such as the @deprecated tag and others. These aren't strictly JavaScript, but given TypeScript's existing interoperability with JSDoc I would assume that it would include them in it's definition of syntax.

A function/method/prop/etc with one of these tags often means something entirely different to the same object without the tag.

Given that the docs for removeComments don't mention it's effect on d.ts output (and explicitly mention only JavaScript), the fact that it has an effect on d.ts is not to be expected (although anyone who's been using TS for long enough will have noticed this unmentioned effect).

I don't think anyone would add a @deprecated tag to something, be it public API or internal, if they didn't mean to say "this object should not be used and will be removed soon".

Given the above 3 statements, I think it's incorrect behavior for TS to strip these syntactically important tags from the emitted d.ts.

As @fatcerberus said, the effect that I believe should happen is removeComments should not effect JSDoc from being emitted in declaration (at least, preserve anything with additional code-related meaning), but should have the current effect on JavaScript. This is closer to the documentation description, and to what many newcomers to TS would expect.

The difference is I am not requesting the feature (nor am I proposing the same solution), I'm reporting a bug where TypeScript is behaving incorrectly with regards to both the docs and fair assumption.
I don't mind the majority of JSDoc being stripped, @public, @readonly, @const and many others have no use case in TypeScript and provide nothing the language doesn't already provide, but things like @deprecated, @throws, @package don't have equivalent syntax in TypeScript and are the only way to demonstrate this intent. If TypeScript strips these items, particularly without mentioning that it will do so, that to me is incorrect behavior.

At least in part, TypeScript is aware of JSDoc tags and their meaning, this could be built upon to include the syntaxes TypeScript doesn't have, and to always exclude them from the effects of removeComments. Equally, all JSDoc could be preserved in declaration, or only when a new compiler option is set.
Whichever solution, it doesn't matter, @deprecated and @throws are syntax that TypeScript alone cannot express, and should be maintained in declaration output.


Apologies for my behavior, I'll refrain from writing issues when I'm in a bad mood.

@RyanCavanaugh
Copy link
Member

Understood. People who have the same concerns about .d.ts size as you have about .js size, or even have concerns about the contents of their .d.ts files, would have equal footing to complain if we changed removeComments to not remove comments (the barest reading of the name, after all). It's not a defect in that regard -- the behavior is extremely longstanding, people can be reasonably assumed to have taken a dependency on it, and isn't something we'd change without a corresponding configuration update.

The worst case scenario would be something like someone having an API key or trade secret in a comment in their private source code, we change removeComments to not change .d.ts emit, and then the next time that person's auto-publish task runs, they leak their API key or other secret. That'd be pretty bad.

I shouldn't have to effect the size of my JS output for my dts output to be syntactically correct.

If you care about the size of your JS output, but aren't running a proper minifier, then I'd argue you really don't care about the size of your JS output. More on this later.

So all-up I don't think we'd be willing to unilaterally change the behavior of .d.ts emit. The stalled nature of #14619, which I think this boils down to in the absence of a unilateral change, comes down to whether TS having yet another commandline flag is really worth it, given that removeComments: false + "run a minifier" quickly achieves the goals of smallest JS + highest-fidelity .d.ts.

Really I'd argue that removeComments should be deprecated in the long-term, since it achieves only partial gains in terms of JS size compared to a real minifier, and has clear downsides in terms of .d.ts output fidelity. Splitting it into removeComments / preserveDeclarationComments is a bit of a half-measure in that regard.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Out of Scope This idea sits outside of the TypeScript language design constraints labels Nov 18, 2022
@typescript-bot
Copy link
Collaborator

This issue has been marked as "Out of Scope" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Out of Scope This idea sits outside of the TypeScript language design constraints Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

4 participants