From 5c13eaf72e549c0f87f8296905dbb3c7d112d808 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Mon, 12 Mar 2018 11:31:36 -0700 Subject: [PATCH 1/3] convertToEs6Module: Avoid replacing entire function --- src/services/codefixes/convertToEs6Module.ts | 53 ++++++++++++------- src/services/textChanges.ts | 43 +++++++++++++-- ...refactorConvertToEs6Module_export_alias.ts | 2 +- ...refactorConvertToEs6Module_export_named.ts | 15 ++++-- ...ToEs6Module_export_namedClassExpression.ts | 14 +++++ ...s6Module_export_namedFunctionExpression.ts | 2 +- ...torConvertToEs6Module_export_referenced.ts | 8 +-- ...vertToEs6Module_expressionToDeclaration.ts | 4 +- 8 files changed, 106 insertions(+), 35 deletions(-) create mode 100644 tests/cases/fourslash/refactorConvertToEs6Module_export_namedClassExpression.ts diff --git a/src/services/codefixes/convertToEs6Module.ts b/src/services/codefixes/convertToEs6Module.ts index b8c78d57827de..6529cb17c0c31 100644 --- a/src/services/codefixes/convertToEs6Module.ts +++ b/src/services/codefixes/convertToEs6Module.ts @@ -114,8 +114,8 @@ namespace ts.codefix { return false; } case SyntaxKind.BinaryExpression: { - const { left, operatorToken, right } = expression as BinaryExpression; - return operatorToken.kind === SyntaxKind.EqualsToken && convertAssignment(sourceFile, checker, statement as ExpressionStatement, left, right, changes, exports); + const { operatorToken } = expression as BinaryExpression; + return operatorToken.kind === SyntaxKind.EqualsToken && convertAssignment(sourceFile, checker, expression as BinaryExpression, changes, exports); } } } @@ -144,7 +144,7 @@ namespace ts.codefix { return convertPropertyAccessImport(name, initializer.name.text, initializer.expression.arguments[0], identifiers); } else { - // Move it out to its own variable statement. + // Move it out to its own variable statement. (This will not be used if `!foundImport`) return createVariableStatement(/*modifiers*/ undefined, createVariableDeclarationList([decl], declarationList.flags)); } }); @@ -177,12 +177,11 @@ namespace ts.codefix { function convertAssignment( sourceFile: SourceFile, checker: TypeChecker, - statement: ExpressionStatement, - left: Expression, - right: Expression, + assignment: BinaryExpression, changes: textChanges.ChangeTracker, exports: ExportRenames, ): ModuleExportsChanged { + const { left, right } = assignment; if (!isPropertyAccessExpression(left)) { return false; } @@ -190,7 +189,7 @@ namespace ts.codefix { if (isExportsOrModuleExportsOrAlias(sourceFile, left)) { if (isExportsOrModuleExportsOrAlias(sourceFile, right)) { // `const alias = module.exports;` or `module.exports = alias;` can be removed. - changes.deleteNode(sourceFile, statement); + changes.deleteNode(sourceFile, assignment.parent); } else { let newNodes = isObjectLiteralExpression(right) ? tryChangeModuleExportsObject(right) : undefined; @@ -198,12 +197,12 @@ namespace ts.codefix { if (!newNodes) { ([newNodes, changedToDefaultExport] = convertModuleExportsToExportDefault(right, checker)); } - changes.replaceNodeWithNodes(sourceFile, statement, newNodes); + changes.replaceNodeWithNodes(sourceFile, assignment.parent, newNodes); return changedToDefaultExport; } } else if (isExportsOrModuleExportsOrAlias(sourceFile, left.expression)) { - convertNamedExport(sourceFile, statement, left.name, right, changes, exports); + convertNamedExport(sourceFile, assignment as BinaryExpression & { left: PropertyAccessExpression }, changes, exports); } return false; @@ -223,7 +222,7 @@ namespace ts.codefix { case SyntaxKind.SpreadAssignment: return undefined; case SyntaxKind.PropertyAssignment: - return !isIdentifier(prop.name) ? undefined : convertExportsDotXEquals(prop.name.text, prop.initializer); + return !isIdentifier(prop.name) ? undefined : convertExportsDotXEquals_replaceNode(prop.name.text, prop.initializer); case SyntaxKind.MethodDeclaration: return !isIdentifier(prop.name) ? undefined : functionExpressionToDeclaration(prop.name.text, [createToken(SyntaxKind.ExportKeyword)], prop); default: @@ -234,14 +233,12 @@ namespace ts.codefix { function convertNamedExport( sourceFile: SourceFile, - statement: Statement, - propertyName: Identifier, - right: Expression, + assignment: BinaryExpression & { left: PropertyAccessExpression }, changes: textChanges.ChangeTracker, exports: ExportRenames, ): void { // If "originalKeywordKind" was set, this is e.g. `exports. - const { text } = propertyName; + const { text } = assignment.left.name; const rename = exports.get(text); if (rename !== undefined) { /* @@ -249,13 +246,13 @@ namespace ts.codefix { export { _class as class }; */ const newNodes = [ - makeConst(/*modifiers*/ undefined, rename, right), + makeConst(/*modifiers*/ undefined, rename, assignment.right), makeExportDeclaration([createExportSpecifier(rename, text)]), ]; - changes.replaceNodeWithNodes(sourceFile, statement, newNodes); + changes.replaceNodeWithNodes(sourceFile, assignment.parent, newNodes); } else { - changes.replaceNode(sourceFile, statement, convertExportsDotXEquals(text, right)); + convertExportsDotXEquals(assignment, sourceFile, changes); } } @@ -303,7 +300,27 @@ namespace ts.codefix { return makeExportDeclaration([createExportSpecifier(/*propertyName*/ undefined, "default")], moduleSpecifier); } - function convertExportsDotXEquals(name: string | undefined, exported: Expression): Statement { + function convertExportsDotXEquals({ left, right, parent }: BinaryExpression & { left: PropertyAccessExpression }, sourceFile: SourceFile, changes: textChanges.ChangeTracker): void { + const name = left.name.text; + if ((isFunctionExpression(right) || isArrowFunction(right) || isClassExpression(right)) && (!right.name || right.name.text === name)) { + // `exports.f = function() {}` -> `export function f() {}` -- Replace `exports.f = ` with `export `, and insert the name after `function`. + changes.replaceRange(sourceFile, { pos: left.getStart(sourceFile), end: right.getStart(sourceFile) }, createToken(SyntaxKind.ExportKeyword), { suffix: " " }); + + if (!right.name) changes.insertName(sourceFile, right, name); + + const semi = findChildOfKind(parent, SyntaxKind.SemicolonToken, sourceFile); + if (semi) changes.deleteNode(sourceFile, semi, { useNonAdjustedEndPosition: true }); + } + else { + // `exports.f = function g() {}` -> `export const f = function g() {}` -- just replace `exports.` with `export const ` + changes.replaceNodeRangeWithNodes(sourceFile, left.expression, findChildOfKind(left, SyntaxKind.DotToken, sourceFile)!, + [createToken(SyntaxKind.ExportKeyword), createToken(SyntaxKind.ConstKeyword)], + { joiner: " ", suffix: " " }); + } + } + + // TODO: GH#22492 this will cause an error if a change has been made inside the body of the node. + function convertExportsDotXEquals_replaceNode(name: string | undefined, exported: Expression): Statement { const modifiers = [createToken(SyntaxKind.ExportKeyword)]; switch (exported.kind) { case SyntaxKind.FunctionExpression: { diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 0a48bcf622806..4f8841cfd33dc 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -100,6 +100,10 @@ namespace ts.textChanges { preserveLeadingWhitespace?: boolean; } + export interface ReplaceWithMultipleNodesOptions extends InsertNodeOptions { + readonly joiner?: string; + } + enum ChangeKind { Remove, ReplaceWithSingleNode, @@ -130,7 +134,7 @@ namespace ts.textChanges { interface ReplaceWithMultipleNodes extends BaseChange { readonly kind: ChangeKind.ReplaceWithMultipleNodes; readonly nodes: ReadonlyArray; - readonly options?: InsertNodeOptions; + readonly options?: ReplaceWithMultipleNodesOptions; } interface ChangeText extends BaseChange { @@ -303,7 +307,7 @@ namespace ts.textChanges { this.replaceRange(sourceFile, getAdjustedRange(sourceFile, startNode, endNode, options), newNode, options); } - private replaceRangeWithNodes(sourceFile: SourceFile, range: TextRange, newNodes: ReadonlyArray, options: InsertNodeOptions = {}) { + private replaceRangeWithNodes(sourceFile: SourceFile, range: TextRange, newNodes: ReadonlyArray, options: ReplaceWithMultipleNodesOptions & ConfigurableStartEnd = {}) { this.changes.push({ kind: ChangeKind.ReplaceWithMultipleNodes, sourceFile, range, options, nodes: newNodes }); return this; } @@ -312,7 +316,7 @@ namespace ts.textChanges { return this.replaceRangeWithNodes(sourceFile, getAdjustedRange(sourceFile, oldNode, oldNode, options), newNodes, options); } - public replaceNodeRangeWithNodes(sourceFile: SourceFile, startNode: Node, endNode: Node, newNodes: ReadonlyArray, options: ChangeNodeOptions = useNonAdjustedPositions) { + public replaceNodeRangeWithNodes(sourceFile: SourceFile, startNode: Node, endNode: Node, newNodes: ReadonlyArray, options: ReplaceWithMultipleNodesOptions & ConfigurableStartEnd = useNonAdjustedPositions) { return this.replaceRangeWithNodes(sourceFile, getAdjustedRange(sourceFile, startNode, endNode, options), newNodes, options); } @@ -320,7 +324,7 @@ namespace ts.textChanges { this.replaceRange(sourceFile, createTextRange(pos), newNode, options); } - private insertNodesAt(sourceFile: SourceFile, pos: number, newNodes: ReadonlyArray, options: InsertNodeOptions = {}): void { + private insertNodesAt(sourceFile: SourceFile, pos: number, newNodes: ReadonlyArray, options: ReplaceWithMultipleNodesOptions = {}): void { this.changes.push({ kind: ChangeKind.ReplaceWithMultipleNodes, sourceFile, options, nodes: newNodes, range: { pos, end: pos } }); } @@ -477,6 +481,35 @@ namespace ts.textChanges { return Debug.failBadSyntaxKind(node); // We haven't handled this kind of node yet -- add it } + public insertName(sourceFile: SourceFile, node: FunctionExpression | ClassExpression | ArrowFunction, name: string): void { + Debug.assert(!node.name); + if (node.kind === SyntaxKind.ArrowFunction) { + const arrow = findChildOfKind(node, SyntaxKind.EqualsGreaterThanToken, sourceFile)!; + const lparen = findChildOfKind(node, SyntaxKind.OpenParenToken, sourceFile); + if (lparen) { + // `() => {}` --> `function f() {}` + this.insertNodesAt(sourceFile, lparen.getStart(sourceFile), [createToken(SyntaxKind.FunctionKeyword), createIdentifier(name)], { joiner: " " }); + this.deleteNode(sourceFile, arrow); + } + else { + // `x => {}` -> `function f(x) {}` + this.insertNodesAt(sourceFile, first(node.parameters).getStart(sourceFile), [createToken(SyntaxKind.FunctionKeyword), createIdentifier(name + "(")], { joiner: " " }); + // Replaceing full range of arrow to get rid of the leading space -- replace ` =>` with `)` + this.replaceRange(sourceFile, arrow, createToken(SyntaxKind.CloseParenToken)); + } + + if (node.body.kind !== SyntaxKind.Block) { + // `() => 0` => `function f() { return 0; }` + this.insertNodesAt(sourceFile, node.body.getStart(sourceFile), [createToken(SyntaxKind.OpenBraceToken), createToken(SyntaxKind.ReturnKeyword)], { joiner: " ", suffix: " " }); + this.insertNodesAt(sourceFile, node.body.end, [createToken(SyntaxKind.SemicolonToken), createToken(SyntaxKind.CloseBraceToken)], { joiner: " " }); + } + } + else { + const pos = findChildOfKind(node, node.kind === SyntaxKind.FunctionExpression ? SyntaxKind.FunctionKeyword : SyntaxKind.ClassKeyword, sourceFile)!.end; + this.insertNodeAt(sourceFile, pos, createIdentifier(name), { prefix: " " }); + } + } + /** * This function should be used to insert nodes in lists when nodes don't carry separators as the part of the node range, * i.e. arguments in arguments lists, parameters in parameter lists etc. @@ -646,7 +679,7 @@ namespace ts.textChanges { const { options = {}, range: { pos } } = change; const format = (n: Node) => getFormattedTextOfNode(n, sourceFile, pos, options, newLineCharacter, formatContext, validate); const text = change.kind === ChangeKind.ReplaceWithMultipleNodes - ? change.nodes.map(n => removeSuffix(format(n), newLineCharacter)).join(newLineCharacter) + ? change.nodes.map(n => removeSuffix(format(n), newLineCharacter)).join(change.options.joiner || newLineCharacter) : format(change.node); // strip initial indentation (spaces or tabs) if text will be inserted in the middle of the line const noIndent = (options.preserveLeadingWhitespace || options.indentation !== undefined || getLineStartPositionForPosition(pos, sourceFile) === pos) ? text : text.replace(/^\s+/, ""); diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_alias.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_alias.ts index 73cd606adf949..8749488b88e47 100644 --- a/tests/cases/fourslash/refactorConvertToEs6Module_export_alias.ts +++ b/tests/cases/fourslash/refactorConvertToEs6Module_export_alias.ts @@ -10,6 +10,6 @@ verify.codeFix({ description: "Convert to ES6 module", newFileContent: ` -export function f() { } +export function f() {} `, }); diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_named.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_named.ts index 6f4c3ecbde4d0..60d2436ddb06e 100644 --- a/tests/cases/fourslash/refactorConvertToEs6Module_export_named.ts +++ b/tests/cases/fourslash/refactorConvertToEs6Module_export_named.ts @@ -6,6 +6,10 @@ ////[|exports.f = function() {}|]; ////exports.C = class {}; ////exports.x = 0; +////exports.a1 = () => {}; +////exports.a2 = () => 0; +////exports.a3 = x => { x; }; +////exports.a4 = x => x; verify.getSuggestionDiagnostics([{ message: "File is a CommonJS module; it may be converted to an ES6 module.", @@ -15,8 +19,11 @@ verify.getSuggestionDiagnostics([{ verify.codeFix({ description: "Convert to ES6 module", newFileContent: -`export function f() { } -export class C { -} -export const x = 0;`, +`export function f() {} +export class C {} +export const x = 0; +export function a1() {} +export function a2() { return 0; } +export function a3(x) { x; } +export function a4(x) { return x; }`, }); diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_namedClassExpression.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_namedClassExpression.ts new file mode 100644 index 0000000000000..ff14f00867d96 --- /dev/null +++ b/tests/cases/fourslash/refactorConvertToEs6Module_export_namedClassExpression.ts @@ -0,0 +1,14 @@ +/// + +// @allowJs: true + +// @Filename: /a.js +////exports.C = class E { static instance = new E(); } +////exports.D = class D { static instance = new D(); } + +verify.codeFix({ + description: "Convert to ES6 module", + newFileContent: +`export const C = class E { static instance = new E(); } +export class D { static instance = new D(); }`, +}); diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_namedFunctionExpression.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_namedFunctionExpression.ts index dbd4e6a0a4cb2..94a14d1a4c350 100644 --- a/tests/cases/fourslash/refactorConvertToEs6Module_export_namedFunctionExpression.ts +++ b/tests/cases/fourslash/refactorConvertToEs6Module_export_namedFunctionExpression.ts @@ -9,6 +9,6 @@ verify.codeFix({ description: "Convert to ES6 module", newFileContent: -`export const f = function g() { g(); }; +`export const f = function g() { g(); } export function h() { h(); }`, }); diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_referenced.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_referenced.ts index e4b4c34f86a4e..7d48b5de71c28 100644 --- a/tests/cases/fourslash/refactorConvertToEs6Module_export_referenced.ts +++ b/tests/cases/fourslash/refactorConvertToEs6Module_export_referenced.ts @@ -12,13 +12,14 @@ //// ////exports.z = 2; ////exports.f = function(z) { -//// z; +//// exports.z; z; ////} // TODO: GH#22492 Should be a able access `exports.z` inside `exports.f` verify.codeFix({ description: "Convert to ES6 module", + // TODO: GH#22492 newFileContent: `export const x = 0; x; @@ -28,8 +29,9 @@ const _y = y; export { _y as y }; _y; -export const z = 2; +const _z = 2; +export { _z as z }; export function f(z) { - z; + _z; z; }`, }); diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_expressionToDeclaration.ts b/tests/cases/fourslash/refactorConvertToEs6Module_expressionToDeclaration.ts index 0525a2b881cca..e22aa1767381d 100644 --- a/tests/cases/fourslash/refactorConvertToEs6Module_expressionToDeclaration.ts +++ b/tests/cases/fourslash/refactorConvertToEs6Module_expressionToDeclaration.ts @@ -10,7 +10,5 @@ verify.codeFix({ description: "Convert to ES6 module", newFileContent: `export async function* f(p) { p; } -export class C extends D { - m() { } -}`, +export class C extends D { m() {} }`, }); From a78a41c84aca33b4ec672f927df0416865134c65 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Thu, 12 Apr 2018 11:38:35 -0700 Subject: [PATCH 2/3] Code review --- src/services/codefixes/convertToEs6Module.ts | 4 ++-- src/services/textChanges.ts | 4 ++-- .../fourslash/refactorConvertToEs6Module_export_referenced.ts | 1 - 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/services/codefixes/convertToEs6Module.ts b/src/services/codefixes/convertToEs6Module.ts index 6529cb17c0c31..be6ff5e939989 100644 --- a/src/services/codefixes/convertToEs6Module.ts +++ b/src/services/codefixes/convertToEs6Module.ts @@ -252,7 +252,7 @@ namespace ts.codefix { changes.replaceNodeWithNodes(sourceFile, assignment.parent, newNodes); } else { - convertExportsDotXEquals(assignment, sourceFile, changes); + convertExportsPropertyAssignment(assignment, sourceFile, changes); } } @@ -300,7 +300,7 @@ namespace ts.codefix { return makeExportDeclaration([createExportSpecifier(/*propertyName*/ undefined, "default")], moduleSpecifier); } - function convertExportsDotXEquals({ left, right, parent }: BinaryExpression & { left: PropertyAccessExpression }, sourceFile: SourceFile, changes: textChanges.ChangeTracker): void { + function convertExportsPropertyAssignment({ left, right, parent }: BinaryExpression & { left: PropertyAccessExpression }, sourceFile: SourceFile, changes: textChanges.ChangeTracker): void { const name = left.name.text; if ((isFunctionExpression(right) || isArrowFunction(right) || isClassExpression(right)) && (!right.name || right.name.text === name)) { // `exports.f = function() {}` -> `export function f() {}` -- Replace `exports.f = ` with `export `, and insert the name after `function`. diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 4f8841cfd33dc..6beb24145882d 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -493,8 +493,8 @@ namespace ts.textChanges { } else { // `x => {}` -> `function f(x) {}` - this.insertNodesAt(sourceFile, first(node.parameters).getStart(sourceFile), [createToken(SyntaxKind.FunctionKeyword), createIdentifier(name + "(")], { joiner: " " }); - // Replaceing full range of arrow to get rid of the leading space -- replace ` =>` with `)` + this.insertText(sourceFile, first(node.parameters).getStart(sourceFile), `function ${name} (`); + // Replacing full range of arrow to get rid of the leading space -- replace ` =>` with `)` this.replaceRange(sourceFile, arrow, createToken(SyntaxKind.CloseParenToken)); } diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_referenced.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_referenced.ts index 7d48b5de71c28..0811ea13e998f 100644 --- a/tests/cases/fourslash/refactorConvertToEs6Module_export_referenced.ts +++ b/tests/cases/fourslash/refactorConvertToEs6Module_export_referenced.ts @@ -19,7 +19,6 @@ verify.codeFix({ description: "Convert to ES6 module", - // TODO: GH#22492 newFileContent: `export const x = 0; x; From 2415f2f97dea0579820b6b7b0b0e63c67cb9d1ac Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Mon, 16 Apr 2018 12:59:27 -0700 Subject: [PATCH 3/3] Fix typo --- src/services/textChanges.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts index 6beb24145882d..ce25bd4e9af17 100644 --- a/src/services/textChanges.ts +++ b/src/services/textChanges.ts @@ -493,7 +493,7 @@ namespace ts.textChanges { } else { // `x => {}` -> `function f(x) {}` - this.insertText(sourceFile, first(node.parameters).getStart(sourceFile), `function ${name} (`); + this.insertText(sourceFile, first(node.parameters).getStart(sourceFile), `function ${name}(`); // Replacing full range of arrow to get rid of the leading space -- replace ` =>` with `)` this.replaceRange(sourceFile, arrow, createToken(SyntaxKind.CloseParenToken)); }