Preserve method comments in JS->ES6 conversion. #16697
Conversation
| if (isSourceFileJavaScript(sourceFile)) { | ||
| return; | ||
| } | ||
| return createProperty(/*decorators*/ undefined, modifiers, memberDeclaration.name, /*questionToken*/ undefined, |
There was a problem hiding this comment.
Should we copy comments for properties as well?
|
@rbuckton review latest rev? |
rbuckton
left a comment
rbuckton
left a comment
There was a problem hiding this comment.
Can you add tests for the arrow and property declaration cases?
| bodyBlock = createBlock([createReturn(expression)]); | ||
| } | ||
| return createMethod(/*decorators*/ undefined, modifiers, /*asteriskToken*/ undefined, memberDeclaration.name, /*questionToken*/ undefined, | ||
| const method = createMethod(/*decorators*/ undefined, modifiers, /*asteriskToken*/ undefined, memberDeclaration.name, /*questionToken*/ undefined, |
There was a problem hiding this comment.
Can you add a test for this?
There was a problem hiding this comment.
Not related to comments, but do we preserve the binding to the this of the outer scope if the arrow contains a this? What happens if we refactor:
function C() {}
C.prototype.x = () => this;| return; | ||
| } | ||
| return createProperty(/*decorators*/ undefined, modifiers, memberDeclaration.name, /*questionToken*/ undefined, | ||
| const prop = createProperty(/*decorators*/ undefined, modifiers, memberDeclaration.name, /*questionToken*/ undefined, |
There was a problem hiding this comment.
Can you add a test for this?
There was a problem hiding this comment.
There was a problem hiding this comment.
That only hits the code path on L167. It doesn't cover the cases on L186 and L189.
There was a problem hiding this comment.
This is actually currently unreachable since this refactor doesn't light up in non-JS files
There was a problem hiding this comment.
Fair point regarding the property declaration, but arrow functions are still fair game.
rbuckton
left a comment
rbuckton
left a comment
There was a problem hiding this comment.
Looks good, just one question.
| bodyBlock = createBlock([createReturn(expression)]); | ||
| } | ||
| return createMethod(/*decorators*/ undefined, modifiers, /*asteriskToken*/ undefined, memberDeclaration.name, /*questionToken*/ undefined, | ||
| const method = createMethod(/*decorators*/ undefined, modifiers, /*asteriskToken*/ undefined, memberDeclaration.name, /*questionToken*/ undefined, |
There was a problem hiding this comment.
Not related to comments, but do we preserve the binding to the this of the outer scope if the arrow contains a this? What happens if we refactor:
function C() {}
C.prototype.x = () => this;
|
We don't preserve arrow functions' |
|
Thanks! |
|
Can you merge this into release-2.4? |
3 participants
Fixes #16622
cc @rbuckton