Do not emit /// comments when remove comments is specified#3883
Conversation
There was a problem hiding this comment.
Remove the unnecessary (
There was a problem hiding this comment.
also move the RHS of the && to its own line
There was a problem hiding this comment.
I do not think this is the right fix. the method is called isPinnedOrTripleSlashComment yet it is isPennedOrSomeTimesTripleSlashComments.
The question is why are we emitting tripple slash comments.. other than the top of the file, why do we care?
There was a problem hiding this comment.
I agree with mohamed. I think it should be something like "shoudlEmitComment" and it can take a boolean saying if we're before the first token or not. Then:
- We always emit pinned comments.
- If we're before the first comment then we always emit triple-slash comments.
- If we're after the first comment and it is not pinned, then we emit based on hte value of removeComments.
Conflicts: src/compiler/emitter.ts src/compiler/scanner.ts
…removeComment is true
Conflicts: src/compiler/emitter.ts
There was a problem hiding this comment.
nit: use braces, and split on 2 lines.
There was a problem hiding this comment.
leading comments can be undefined here. we check for it later. no need for the empty array literal allocation
|
LGTM. @sheetalkamat can you take a look. |
There was a problem hiding this comment.
Instead of doing this can you make emitLeadingCommentsOfPosition a function pointer. It helps with the perf since that check happens just once. Do this for other functions that do nothing when remove comments too.
emitLeadingCommentsOfPosition = compilerOptions.removeComments ? function (pos: number) { } : emitLeadingOCmmentsOfPositionWorker|
@yui can you recap where we landed on this? have we written up the set of rules we'll follow for the different types of comments and the different compiler options? I want there to be a crisp set of guidelines we can point customers at, and which we can use to validate our behavior against moving forward. Thanks! |
|
Let's put the rules in the wiki somewhere |
|
@CyrusNajmabadi Sorry for not updating the issue with the new rules set. I will put the rules we landed in the wiki for more visibility as well as update the issue. But just to recap the decision that has been made. |
|
@yuit Has F1 signed off on this? Weren't they the ones that wanted /// comments to be kept, even with --removeComments:true? |
This doesn't make sense to me. Pinned comments should always be kept (regardless of where they are). The reason people use pinned comments is precisely so they'll be kept, even if the presence of general comment stripping mechanism. That's how they're used in other tools that support them. This is also important for cases like like angular. this allows people to put in comments like Is there a reason we would have this strange support for pinned comments where we only keep them around some of the time? Won't that just be very confusing for users? I'm also not getting how it's helpful just to keep them for the top of a file. Is there data that shows that this is really the only place where people put these comments? |
I don't understand how this simplifies things. Indeed, now you'll need logic that says "if this is a pinned comment at the top of the file, then keep it. Otherwise, remove the pinned comment." I think it's simpler to just say "if it's a pinned comment, always keep it" :-) |
There was a problem hiding this comment.
return currentSourceFile.text.charCodeAt(comment.pos + 1) === CharacterCodes.asterisk &&
currentSourceFile.text.charCodeAt(comment.pos + 2) === CharacterCodes.exclamation;|
I talked about this with Yui. To me, if this is the direction we're going, it's unclear to me why we're adding any support at all for 'pinned comments'? If the only pinned comments we keep are the ones at hte very top of the file, then it seems like we're basically not really implementing the feature for people who want it. That's fine to me. But i would just not have any support for pinned comments at all at that point. Why have the complexity to support one single case? If people want pinned comments to be preserved, we can just tell them "don't use --removeComments" and then use some other tool. Also, it seems like we could simplify even further by just removing support for --removeComments in the first place. It seems like most people who care about JS size will just use a minifier, and that will take care of comments for them. This means removing all complexity in the emitter around comments entirely. I'd be ok with this being the route we go. But this middle ground seems nonsensical to me. |
|
My understanding was removeComments should just do what it says: remove all comments. Except we made one exception for copyright headers at the top of source files (the only form of pinned comments) because copyrights are pretty important to keep around. Trying to get fancier than that leads to the path you described: people can just go get a 'real' minifier or write your own postprocess step to strip // but not /// or whatever your rule is. Certainly there is an argument for removing it entirely and saying 'go get a minifier' but we did originally do removeComments based on feedback from people who didn't want to add a full minification post-build step but did want some basic support for stripping what was truly unnecessary. |
|
@danquirk We added pinned comment (literally the |
|
@CyrusNajmabadi . Confirmed with F12, they no longer need support for ///. One possible solution is to always emit header comments (since the comments are likely to be copy-right) whether the comments are prefix with pinned comment notation Edit: Open an issue #4734. |
Do not emit /// comments when remove comments is specified
Issue #3283.