[Master] Fix 13602 preserve comment following element in node list#13916
Conversation
| emitNodeWithComments, | ||
| emitBodyWithDetachedComments, | ||
| emitTrailingCommentsOfPosition, | ||
| emitLeadingComments, |
There was a problem hiding this comment.
Do not expose emitLeadingComments, but rather create an emitLeadingCommentsOfPosition that calls emitLeadingComments with true for isEmittedNode. The isEmittedNode flag only matters for nodes that are not emitted at the top of a file (such as an interface) and shouldn't matter for any of the cases in emitter.ts.
There was a problem hiding this comment.
We should ensure that emitLeadingCommentsOfPosition does nothing if pos is -1.
There was a problem hiding this comment.
Thanks @rbuckton ! 🍰 I was wondering if I should create similar function call like emitTrailingCommentsOfPosition
| } | ||
|
|
||
| function emitLeadingComments(pos: number, isEmittedNode: boolean) { | ||
| if (disabled) { |
There was a problem hiding this comment.
Move this into an emitLeadingCommentsOfPosition function that calls emitLeadingComments (see above).
| // ]; | ||
| if (previousSibling && delimiter && previousSibling.end !== parentNode.end) { | ||
| emitLeadingComments(previousSibling.end, /*isEmittedNode*/ previousSibling.kind !== SyntaxKind.NotEmittedStatement); | ||
| if (hasTrailingComma) { |
There was a problem hiding this comment.
Why are you emitting this twice? Also, statements are never emitted with delimiters, so your isEmittedNode argument will always be true.
| if (commentedParameters(1, 2)) { | ||
| /*comment1*/ | ||
| commentedParameters(3, 4); | ||
| /*comment2*/ |
There was a problem hiding this comment.
This should be indented similar to /*comment1*/ above.
There was a problem hiding this comment.
We might want to investigate whether it makes sense to emit leading/trailing comments of a position in the emitNodeList function in emitter.ts.
There was a problem hiding this comment.
not sure I understand your comment. do you emit leading/trailing comments of each element in node-list?
| // function commentedParameters( | ||
| // /* Parameter a */ | ||
| // a | ||
| // /* End of parameter a */ -> this comment doesn't consider to be trailing comment of parameter "a" due to newline |
There was a problem hiding this comment.
doesn't consider -> isn't considered
| writeToken(SyntaxKind.OpenBraceToken, node.pos, /*contextNode*/ node); | ||
| emitBlockStatements(node); | ||
| // We have to call emitLeadingComments explicitly here because otherwise leading comments of the close brace token will not be emitted | ||
| emitLeadingComments(node.statements.end, /*isEmittedNode*/true); |
There was a problem hiding this comment.
Perhaps we should emit leading/trailing comments of the node range in emitNodeList
There was a problem hiding this comment.
do you mean make a call like this emitLeadingCommentsOfPosition(nodeList.pos)?
| } | ||
| C.prototype.P = function (ii, j, k) { | ||
| for (var i = 0; i < arguments.length; i++) { | ||
| // WScript.Echo("param: " + arguments[i]); |
| define(["require", "exports", "./foo_0"], function (require, exports, foo) { | ||
| "use strict"; | ||
| if (foo.E1.A === 0) { | ||
| // Should cause runtime import - interesting optimization possibility, as gets inlined to 0. |
| // a | ||
| // /* End of parameter a */ -> this comment isn't considered to be trailing comment of parameter "a" due to newline | ||
| // , | ||
| if (emitLeadingCommentsOfPosition && delimiter && previousSibling.end !== parentNode.end) { |
There was a problem hiding this comment.
There shouldn't be a need to test for the existence of emitLeadingCommentsOfPosition.
Fix issue #13602.
tsc with no comment flags remove some comments. #13602 (comment)
tsc with no comment flags remove some comments. #13602 (comment)
Wont fix at the moment
- [ ] #13602 (comment)- [ ] #13602 (comment)