Skip to content

Preserve method comments in JS->ES6 conversion. #16697

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.

Sign up for GitHub

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

Merged
RyanCavanaugh merged 3 commits into from
Jun 29, 2017
Merged

Preserve method comments in JS->ES6 conversion. #16697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
04d750f
Preserve method comments in JS->ES6 conversion. Fixes #16622
RyanCavanaugh Jun 22, 2017
b52747e
Add property comments as well
RyanCavanaugh Jun 26, 2017
277f459
Add tests
RyanCavanaugh Jun 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 38 additions & 8 deletions src/services/refactors/convertFunctionToEs6Class.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,12 +164,15 @@ namespace ts.refactor {
}

switch (assignmentBinaryExpression.right.kind) {
case SyntaxKind.FunctionExpression:
case SyntaxKind.FunctionExpression: {
const functionExpression = assignmentBinaryExpression.right as FunctionExpression;
return createMethod(/*decorators*/ undefined, modifiers, /*asteriskToken*/ undefined, memberDeclaration.name, /*questionToken*/ undefined,
const method = createMethod(/*decorators*/ undefined, modifiers, /*asteriskToken*/ undefined, memberDeclaration.name, /*questionToken*/ undefined,
/*typeParameters*/ undefined, functionExpression.parameters, /*type*/ undefined, functionExpression.body);
copyComments(assignmentBinaryExpression, method);
return method;
}

case SyntaxKind.ArrowFunction:
case SyntaxKind.ArrowFunction: {
const arrowFunction = assignmentBinaryExpression.right as ArrowFunction;
const arrowFunctionBody = arrowFunction.body;
let bodyBlock: Block;
Expand All @@ -183,20 +186,42 @@ namespace ts.refactor {
const expression = arrowFunctionBody as Expression;
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,

@rbuckton rbuckton Jun 29, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test for this?

@rbuckton rbuckton Jun 29, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

/*typeParameters*/ undefined, arrowFunction.parameters, /*type*/ undefined, bodyBlock);
copyComments(assignmentBinaryExpression, method);
return method;
}

default:
default: {
// Don't try to declare members in JavaScript files
if (isSourceFileJavaScript(sourceFile)) {
return;
}
return createProperty(/*decorators*/ undefined, modifiers, memberDeclaration.name, /*questionToken*/ undefined,
const prop = createProperty(/*decorators*/ undefined, modifiers, memberDeclaration.name, /*questionToken*/ undefined,

@rbuckton rbuckton Jun 29, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test for this?

@RyanCavanaugh RyanCavanaugh Jun 29, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/Microsoft/TypeScript/pull/16697/files#diff-5cfc913717c53d115c9368927495ac25R6

@rbuckton rbuckton Jun 29, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That only hits the code path on L167. It doesn't cover the cases on L186 and L189.

@RyanCavanaugh RyanCavanaugh Jun 29, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually currently unreachable since this refactor doesn't light up in non-JS files

@rbuckton rbuckton Jun 29, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point regarding the property declaration, but arrow functions are still fair game.

/*type*/ undefined, assignmentBinaryExpression.right);
copyComments(assignmentBinaryExpression.parent, prop);
return prop;
}
}
}
}

function copyComments(sourceNode: Node, targetNode: Node) {
forEachLeadingCommentRange(sourceFile.text, sourceNode.pos, (pos, end, kind, htnl) => {
if (kind === SyntaxKind.MultiLineCommentTrivia) {
// Remove leading /*
pos += 2;
// Remove trailing */
end -= 2;
}
else {
// Remove leading //
pos += 2;
}
addSyntheticLeadingComment(targetNode, kind, sourceFile.text.slice(pos, end), htnl);
});
}

function createClassFromVariableDeclaration(node: VariableDeclaration): ClassDeclaration {
const initializer = node.initializer as FunctionExpression;
if (!initializer || initializer.kind !== SyntaxKind.FunctionExpression) {
Expand All @@ -212,17 +237,22 @@ namespace ts.refactor {
memberElements.unshift(createConstructor(/*decorators*/ undefined, /*modifiers*/ undefined, initializer.parameters, initializer.body));
}

return createClassDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, node.name,
const cls = createClassDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, node.name,
/*typeParameters*/ undefined, /*heritageClauses*/ undefined, memberElements);
// Don't call copyComments here because we'll already leave them in place
return cls;
}

function createClassFromFunctionDeclaration(node: FunctionDeclaration): ClassDeclaration {
const memberElements = createClassElementsFromSymbol(ctorSymbol);
if (node.body) {
memberElements.unshift(createConstructor(/*decorators*/ undefined, /*modifiers*/ undefined, node.parameters, node.body));
}
return createClassDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, node.name,

const cls = createClassDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, node.name,
/*typeParameters*/ undefined, /*heritageClauses*/ undefined, memberElements);
// Don't call copyComments here because we'll already leave them in place
return cls;
}
}
}
45 changes: 45 additions & 0 deletions tests/cases/fourslash/convertFunctionToEs6ClassJsDoc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/// <reference path='fourslash.ts' />

// @allowNonTsExtensions: true
// @Filename: test123.js
//// function fn() {
//// /** neat! */
//// this.x = 100;
//// }
////
//// /** awesome
//// * stuff
//// */
//// fn.prototype.arr = () => { return ""; }
//// /** great */
//// fn.prototype.arr2 = () => [];
////
//// /**
//// * This is a cool function!
//// */
//// /*1*/fn.prototype.bar = function (x, y, z) {
//// this.x = y;
//// };

verify.fileAfterApplyingRefactorAtMarker('1',
`class fn {
constructor() {
/** neat! */
this.x = 100;
}
/** awesome
* stuff
*/
arr() { return ""; }
/** great */
arr2() { return []; }
/**
* This is a cool function!
*/
bar(x, y, z) {
this.x = y;
}
}


`, 'Convert to ES2015 class', 'convert');