Typescript: removeComments compiler option seems to be ignored when using typescript 1.5.0-beta

[milkshakeuk]

Specifying the argument ($ tsc --removeComments) or option in (tsconfig.json) for removing comments is not honoured.

Normal comments are removed, but special comments, like references and amd dependencies, are preserved.

/// <reference path="../Interfaces/IView" />
/// <reference path="./BaseView" />
/// <amd-dependency path="/js/libs/hgn.js!app/templates/home" name="compiler"/>

I don't know if this new behaviour is intended but this wasn't the case in the previous release and I wouldn't expect these special comments to be required in the output files.

[NickHeiner]

Additionally, I'm seeing multiline comments not getting removed:

index.ts

'use strict';

// single line comment
console.log('hi');

/**
 * js-doc style comment
 */
function foo() {
    /*

        multi line
        comment
    */

    return 42;
}

tsconfig.json

{
    "compilerOptions": {
        "removeComments": true
    }
}

λ tsc -v
message TS6029: Version 1.5.3
λ tsc -p .

The output js:

'use strict';
console.log('hi');
function foo() {
    /*

        multi line
        comment
    */
    return 42;
}

[NickHeiner]

Another (possibly related) bug: multiline comments are _always_ removed if they are the only thing in thing in the body of a function:

tsconfig.json

{
    "compilerOptions": {
        "module": "commonjs",
        "removeComments": false
    }
}

index.ts

function foo() { /*
    This multiline string will only work
    if the TS compiler does not remove comments.
*/ }

λ tsc -v
message TS6029: Version 1.5.3
λ tsc -p .

Generated index.js

function foo() {
}

If I add a statement to the body of foo, then I do not see this problem.

This breaks code that uses the multiline module.

[NickHeiner]

And I know that TS offers multiline strings via template strings, but here's another use case:

index.ts

declare var angular: any;

angular.module('app').factory('myService', /* @ngInject */ function foo($scope, filter) {

});

tsconfig.json

{
    "compilerOptions": {
        "removeComments": false
    }
}

λ tsc -v
message TS6029: Version 1.5.3
λ tsc -p .

Generated index.js

angular.module('app').factory('myService', function foo($scope, filter) {
});

The /* @ngInject */ comment needs to be there so ng-annotate can annotate the Angular dependency injection function.

[mhegazy]

@NickHeiner the last issue seems to be a different one. I have filed #3844 to track it.

[NickHeiner]

Awesome, thanks!

[DickvdBrink]

@yuit, Can I take this one?

[yuit]

@DickvdBrink sure. Thanks!
Edit: I fix the original issue on removing triple-slash comment (PR is coming out soon). However, I haven't look at other issues in the comments which seem to require separate fix from the original one. If you could take a look on that, it will be great!

[yuit]

An update on this issue, @CyrusNajmabadi explains /// comment is a special directive in our language not just a plain comment and thus we should keep it. I have opened an issue regarding fine-tuning removing of "comments" issue #3878 that we can discuss further.

Thus, we will keep currently behavior of the /// comments (i.e. we will keep /// comments) However, following case is a bug that we will have to fix (when /// comments are not at the top of the file.)

var x = 10
/// <reference path ="blah.ts" />

[GaryChamberlain]

I'm seeing this too. It really bloats the size of my outputted js file. After switching to tsc 1.5.3 my js file is 15% bigger due to comments!!

The /// comment is a special directive in the ts file but not in the outputted js file. I'm concatenating to a single file and those things are now all over the non-minified js file. If you decide to not have --removeComments remove them, like it did before v1.5, then please add a separate mechanism to remove those directive comments. Please!!

[milkshakeuk]

@yuit why would the /// comments need to be preserved in the output js file?
As far as I'm aware IDE's or text editors like vscode may use these to provide code intelligence at develop time (even in js) but in regards to the code which you will deploy I can't think of any reason for the /// comments to remain.

[DickvdBrink]

For exemple Copyrights comments need to be preserved in some cases. (I believe that was one of the points if I recall correctly)

[milkshakeuk]

is there no way to tell the difference between copyrights and special xml elements like:

/// <reference path="../Interfaces/IView" />
/// <reference path="./BaseView" />
/// <amd-dependency path="/js/libs/hgn.js!app/templates/home" name="compiler"/>

I can't imagine that to be the case.

[CyrusNajmabadi]

There are a lot of things being conflated here. Let's run down the list of special constructs that TS knows about:

1. 'Detached' comments. These are at the start of the file or function and must have at least one blank line between them and the next construct.
2. 'Directive' comments. These start with /// < and have some well know name after the open angle (like reference or amd-module.
3. 'Pinned' comments. i.e. comments that start with /*!.
4. 'Normal' comments. i.e. any other type of comment (these start with /* or //).

Currently, thanks to the work by @yuit, --removeComments should only affect the last comment type. In other words, the presence of this flag only affects if we emit 'Normal' comments. These are the general comments that make up the majority of comments in a project, and which add the most to output size. They also have no meaning except to the developer, and thus can be removed safely, and have no meaning to other tools.

Currently --removeComments does not remove the first three comment types, and we will attempt to preserve them regardless of this flag. These reason for this is as follows:

1. 'Detached' comments often indicate a copyright declaration of some sort. Developers often want these transferred over, even if they don't want regular comments. So we're keeping those by default.
2. 'Directive' comments are important to some customers. We have customers who use reference directive in later parts of their JS processing pipeline. So we're also keeping those by default.
3. 'Pinned' comments literally exist to tell tooling 'do not remove me', so we should always keep these no matter what**.

Now, we recognize that some users may want further comment removal. Specifically removal of 'Detached' or 'Directive' comments (again, we will always try to preserve 'Pinned' comments**). When @yuit did this work we discussed this need and speculated that we would have further options to the compiler to allow customization here. We'll be keeping the --removeComments flag and keeping the new default behavior. However, we are also considering adding things like (final name/form TBD):

removeSpecialComments=detached,directive

With such an option you can opt out of our default behavior and trim your files even further. We did not do this originally because we weren't even certain there would be anyone who would want/need this. And investing in this work based on speculative need seemed unnecessary. Now that it's clearer that there are some people who might want this, we will revisit and potentially reconsider this decision.

** Currently pinned comments are not emitted in some cases. However, i consider that a bug. We should always be emitting pinned comments in all circumstances. That's the point of the construct in the first place. Fortunately, this bug doesn't seem to have that much impact. The places where people use pinned comments do not seem to the be the same as the places where we accidentally do not emit them.

[CyrusNajmabadi]

Note: i do find the concerns about file size somewhat interesting. Not that people are concerned about file sizes themselves. That i understand. But that people are concerned with that aspect, but are not using a minifier. If file size is a concern, why not use a minifier (which will then appropriate remove comments, as well as a whole host of other shrinking techniques)?

It seems like if you're not using a minifier, then you're already going to be getting .js that is an order of magnitude larger than necessary.

@GaryChamberlain

After switching to tsc 1.5.3 my js file is 15% bigger due to comments!!

Hi gary, could you let us know why you're not using a minifier for your code if size is a concern? Thanks!

@milkshakeuk

but in regards to the code which you will deploy I can't think of any reason for the /// comments to remain.

The TS compiler doesn't know what stage in your pipeline it is involved in. We may be the last stage before you finally deploy. Or we may just be an initial stage. As such, we need to preserve important comments so later phases can use them (we have customers that depend on this). If file size is important to you, could you let us know why you're not using a minifier? And, if file size is not important, then why are the special comments an impact to you?

Thanks!

[GaryChamberlain]

@CyrusNajmabadi

Hi gary, could you let us know why you're not using a minifier for your code if size is a concern? Thanks!

Yes, file size is a big concern, and I most definitely _do_ use a minifier (Google Closure). The problem is that the amount any minifier can shrink a js file is a function of how the unminified js is structured. I have a very large TypeScript project under development (see http://pro.style) which has over 250 TS files in a structured namespace hierarchy. I use grunt-ts to compile them all and concatenate them into a single js file.

Personally, I think TypeScript is the best thing since sliced bread, but the outputted js file is extremely wasteful. So, I use a grunt task to clean it up. Using grunt-text-replace and some regex, I search for and delete the hundreds of line blocks that look like this.


})(ProStyle || (ProStyle = {}));

var ProStyle;

(function(ProStyle) {


One of these is emitted between every original .ts file! I only need the partials at the top and bottom of the output. Similarly, I do that for all the child namespaces. It cuts the .js file down by 35% and also has a big impact on the .min.js file. It works like a charm!

Now I know I'm doing some black magic that might need to be adjusted with future versions of TypeScript, and might even be removed altogether if TypeScript ever makes the output better. However, I didn't expect it to break due to removeComments. The _change_ that was introduced with 1.5 emits a bunch of extra /// directive comments that weren't there in prior versions of TypeScript.

You've made valid arguments for how you believe removeComments should work. And if that was how it originally worked, then great, but it wasn't. To introduce the new feature, I would have preferred to keep the existing removeComments functionality when the value is true, and add a third _object_ value for a-la-cart selection (true, false, {detached=true, directive=false, pinned=false, normal=true}).

---

As a side note, I ended up using grunt-contrib-uglify as a post step with minification disabled, but using it's removeComments and beautfier, which makes the unminified version of the file a lot smaller, even before the comments are stripped. I'm now getting a readable, unminified .js file that is half the size of what TypeScript originally emits.

[CyrusNajmabadi]

@GaryChamberlain

Using grunt-text-replace and some regex, I search for and delete the hundreds of line blocks that look like this.

Interesting. I think that's far enough out of the norm for usage, that i'm not too upset about that breaking :) IMO, if you do want to do this sort of advanced type of code manipulation, the onus is on you to be willing to maintain it in the presence of how we change our emit (since we never guarantee anything there). One way you could right things that would be a bit more resilient would be to just use our compiler API itself to parse the code. Then you could walk the AST and emit it however you wanted (including not having bits like })(ProStyle || (ProStyle = {}));). regex's are super fragile and it's pretty fortunate that it's been working at all :)

Indeed, if you want some sort of specialized emit, then it woudl be great if you wrote up what you prefer our compiler emitted instead of what it currently emits. We could then consider providing a specialized output format if that style made sense. That would be a lot nicer than, effectively, hand munging our output :)

The change that was introduced with 1.5 emits a bunch of extra /// directive comments that weren't there in prior versions of TypeScript.

True. But our goal here is to have defaults that work the best for the majority. It's also important to realize that it's better (IMO) to err on the side of having more data in the final output. If we strip some piece of data, and you need it, then you're SOL. However, if we have some extra data, then you can always remove it if you don't want it.

To introduce the new feature, I would have preferred to keep the existing removeComments functionality when the value is true, and add a third object value for a-la-cart selection (true, false, {detached=true, directive=false, pinned=false, normal=true}).

I think it's better for us to be in a sane starting place. Honestly, the use case of "i go in and run some regex find/replaces on the code" is pretty out there. I absolutely realize that for you, this is clearly an annoying change. And i do think it seems like there is value in providing the knobs for you to have no directive-comment emit at all. However, i think that optimizing the default for your specific use case doesn't make sense to me.

I'll talk this over with @yuit on monday. It should be pretty easy for us to provide a solution for you. In the meantime, if this is a real problem for your case, and updating your regex is not viable, you can either rollback to a previous compiler version, or you can tweak the emitter code in our compiler yourself to have the behavior you want. You'll just have to change the following code in tsc.js:

            function isPinnedOrTripleSlashComment(comment) {
                if (currentSourceFile.text.charCodeAt(comment.pos + 1) === 42) {
                    return currentSourceFile.text.charCodeAt(comment.pos + 2) === 33;
                }
                // add a "return false;" statement here.

This will cause us to filter out directive comments as well. That would be a temp workaround until we gave you a dedicate compiler knob for this advanced scenario.

[GaryChamberlain]

Thanks, I wasn't familiar with the AST option. I'll keep that as something I can reach in the tool belt if needed.

I only explained what I am doing as a response to your question about why I am not using a minifier and my comment about pre-minified size. Yep, I know the way I'm hacking it to make it better is something I need to maintain. I'm prepared to tweak it to take TSC upgrades. However, I'm not cleaning it up in some way special to me. I'm just stripping the huge amount of repeated scaffolding. Since it is always the same pattern, I'd prefer typescript to do it, Statistically significantly smaller .js files would be better for everyone.

I think that's far enough out of the norm for usage, that i'm not too upset about that breaking :) IMO

That's a dangerous way to maintain an API, IMO. You didn't envision my use case. What other use cases are you not aware of when making these types of changes? An argument about having default functionality being the best for most people is a a bit weak for making a functional change to the emitter in a minor version change of TypeScript. Also, I would argue that more people than not don't want the extra cruft.

Either way, I'm not using the TSC removeComments option any longer. I've introduced an extra step to do that, which I've explained below for anyone who wants smaller output from TypeScript.

_Thanks for the great responses!_ While I disagree with you on the approach and stance of this particular issue, it in no way diminishes the _gratitude_ I have for you, your team, and Microsoft for building and maintaining such a useful tool!

---

Anyone interested, here are the steps I am performing to get significantly smaller .js and .min.js files from TypeScript.

• compile with grunt-ts v4.2.0 (with TSC 1.5.3)
• remove comments and beautify (no minification) with grunt-contrib-uglify
• remove the unneeded scaffolding with grunt-contrib-replace
• minify with grunt-closurecompiler
• add banners to both .js and ,min.js with grunt-banner

As of the latest (soon to be released) version of ProStyle, the size with TSC and ClosureCompiler alone are

prostyle.js: 435k
prostyle.min.js: 154k

The size with using grunt-contrib-uglify and grunt-contrib-replace before the closurecompiler are

prostyle.js: 248k
prostyle.min.js: 135k

That's 43% and 12.5% smaller respectively with _exactly_ the same functionality.

[CyrusNajmabadi]

That's a dangerous way to maintain an API, IMO

Our emitted code is not an API. It's an implementation detail. As long as it semantically follows the rules of our language, and executes properly in JS engines, then we can (and likely will) change it over time. We've already done this many times over the life of the project for many different reasons. The use case of "someone will then expect precisely this output, and thus any change may break them" isn't something we're signing up for. :)

This is similar to most other compilers out there. C++ compilers do not give guarantees that they will produce the same stream of assembler instructions. .Net compilers do not guarantee you'll get the same IL stream (indeed, that does change from release to release).

You didn't envision my use case. What other use cases are you not aware of when making these types of changes?

Not every use case is something that is supported, or must be maintained:

:)

That's 43% and 12.5% smaller respectively with exactly the same functionality.

Sounds like a great optimization for the minifiers to start adopting. I would recommend filing bugs on them to detect and improve their minification for these patterns. Also, as mentioned before, you may want to file a suggestion on how we could improve our own emit so that you don't need the extra cleanup step you currently have to run. These seems like the best approach overall, vs regex pattern matching modification of our emitted code. :)

[NickHeiner]

Our emitted code is not an API. It's an implementation detail. As long as it semantically follows the rules of our language, and executes properly in JS engines, then we can (and likely will) change it over time.

:+1: :+1: :+1:

[GaryChamberlain]

Our emitted code is not an API. It's an implementation detail. As long as it semantically follows the rules of our language, and executes properly in JS engines, then we can (and likely will) change it over time.
:+1: :+1::+1:

:-1::-1::-1:

There was a compiler switch that used to work a certain way, but was significantly changed with a point release of the compiler. The compiler is scripted with tools like Gulp and Grunt. It's just semantics, whether you call it an API or not. To me it's an API because you program the build process using those switches.

[CyrusNajmabadi]

@GaryChamberlain

The compiler is scripted with tools like Gulp and Grunt. It's just semantics, whether you call it an API or not. To me it's an API because you program the build process using those switches

We've never guaranteed the results of compilation. No more than the C# compiler guarantees the IL instructions it will emit (regardless of compiler flags specified).

Our emit format has already changed several times since we originally launched TS, and I expect it to change more in the future. It is currently not a supported scenario to take a dependency on the specific output being generated.

I totally get that your use case is affected. But note that your use case is extremely specialized, out of bounds of what we guarantee, and likely better served by another approach altogether*. It's akin to writing a tool that goes in and mucks with .Net IL and then no longer works when the C# compiler changes how they emit IL for the same code.

• Indeed, I think far better approaches include:
• Writing some sort of AST visitor (using our parse tree API) that looks for these patterns you care about and excises them appropriately.
• Requesting a new feature whereby we provide this 'slimmer' emitted code form.

Of the two, I far prefer the latter. Then everyone can benefit from this minimization saving.

[alexsad]

I resolve this doing this:
1-open mnt/files/myproject/node_modules/grunt-typescript/tasks/modules/option.js
and then I replace these lines:
tsOptions: {
//removeComments: prepareRemoveComments(source),

To this---->

tsOptions: {
removeComments: true,
noEmitHelpers:true,

[yuit]

An update on this issue (in this PR #3883), we have redesign the functionality of --removeComment and come to conclusion that the flag will remove all comments except header copyright comments that begins with /* as copyright has additional legal significance. We would like to simplify the removing comment process and users who wish to have more control over the functionality should use additional plug-in.

This change has removed the concept of pinned comment (/!*) in that it will not longer get preserved in emitted js files.

[mhegazy]

The fix should be in today's nightly typescript@next, some 10 hours away from now.

[magemello]

Hi I'm using version 1.8.10 and if you don't leave a new line between the header and the first line of code the header is not preserved...

[magemello]

Also If you don't declare a constructor like the one below the header is not preserved in .d.ts files.
constructor(public el:ElementRef) {
console.log('constructor', el);
}

[devenvexe]

@magemello the whitespace does indeed seem to be a problem. I'll look into it.