Skip to content

Commit aac9ef5

Browse files
author
Andy
authored
convertToEs6Module: Avoid replacing entire function (#22507)
* convertToEs6Module: Avoid replacing entire function * Code review * Fix typo
1 parent a8618a7 commit aac9ef5

8 files changed

+105
-35
lines changed

src/services/codefixes/convertToEs6Module.ts

+35-18
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,8 @@ namespace ts.codefix {
114114
return false;
115115
}
116116
case SyntaxKind.BinaryExpression: {
117-
const { left, operatorToken, right } = expression as BinaryExpression;
118-
return operatorToken.kind === SyntaxKind.EqualsToken && convertAssignment(sourceFile, checker, statement as ExpressionStatement, left, right, changes, exports);
117+
const { operatorToken } = expression as BinaryExpression;
118+
return operatorToken.kind === SyntaxKind.EqualsToken && convertAssignment(sourceFile, checker, expression as BinaryExpression, changes, exports);
119119
}
120120
}
121121
}
@@ -144,7 +144,7 @@ namespace ts.codefix {
144144
return convertPropertyAccessImport(name, initializer.name.text, initializer.expression.arguments[0], identifiers);
145145
}
146146
else {
147-
// Move it out to its own variable statement.
147+
// Move it out to its own variable statement. (This will not be used if `!foundImport`)
148148
return createVariableStatement(/*modifiers*/ undefined, createVariableDeclarationList([decl], declarationList.flags));
149149
}
150150
});
@@ -177,33 +177,32 @@ namespace ts.codefix {
177177
function convertAssignment(
178178
sourceFile: SourceFile,
179179
checker: TypeChecker,
180-
statement: ExpressionStatement,
181-
left: Expression,
182-
right: Expression,
180+
assignment: BinaryExpression,
183181
changes: textChanges.ChangeTracker,
184182
exports: ExportRenames,
185183
): ModuleExportsChanged {
184+
const { left, right } = assignment;
186185
if (!isPropertyAccessExpression(left)) {
187186
return false;
188187
}
189188

190189
if (isExportsOrModuleExportsOrAlias(sourceFile, left)) {
191190
if (isExportsOrModuleExportsOrAlias(sourceFile, right)) {
192191
// `const alias = module.exports;` or `module.exports = alias;` can be removed.
193-
changes.deleteNode(sourceFile, statement);
192+
changes.deleteNode(sourceFile, assignment.parent);
194193
}
195194
else {
196195
let newNodes = isObjectLiteralExpression(right) ? tryChangeModuleExportsObject(right) : undefined;
197196
let changedToDefaultExport = false;
198197
if (!newNodes) {
199198
([newNodes, changedToDefaultExport] = convertModuleExportsToExportDefault(right, checker));
200199
}
201-
changes.replaceNodeWithNodes(sourceFile, statement, newNodes);
200+
changes.replaceNodeWithNodes(sourceFile, assignment.parent, newNodes);
202201
return changedToDefaultExport;
203202
}
204203
}
205204
else if (isExportsOrModuleExportsOrAlias(sourceFile, left.expression)) {
206-
convertNamedExport(sourceFile, statement, left.name, right, changes, exports);
205+
convertNamedExport(sourceFile, assignment as BinaryExpression & { left: PropertyAccessExpression }, changes, exports);
207206
}
208207

209208
return false;
@@ -223,7 +222,7 @@ namespace ts.codefix {
223222
case SyntaxKind.SpreadAssignment:
224223
return undefined;
225224
case SyntaxKind.PropertyAssignment:
226-
return !isIdentifier(prop.name) ? undefined : convertExportsDotXEquals(prop.name.text, prop.initializer);
225+
return !isIdentifier(prop.name) ? undefined : convertExportsDotXEquals_replaceNode(prop.name.text, prop.initializer);
227226
case SyntaxKind.MethodDeclaration:
228227
return !isIdentifier(prop.name) ? undefined : functionExpressionToDeclaration(prop.name.text, [createToken(SyntaxKind.ExportKeyword)], prop);
229228
default:
@@ -234,28 +233,26 @@ namespace ts.codefix {
234233

235234
function convertNamedExport(
236235
sourceFile: SourceFile,
237-
statement: Statement,
238-
propertyName: Identifier,
239-
right: Expression,
236+
assignment: BinaryExpression & { left: PropertyAccessExpression },
240237
changes: textChanges.ChangeTracker,
241238
exports: ExportRenames,
242239
): void {
243240
// If "originalKeywordKind" was set, this is e.g. `exports.
244-
const { text } = propertyName;
241+
const { text } = assignment.left.name;
245242
const rename = exports.get(text);
246243
if (rename !== undefined) {
247244
/*
248245
const _class = 0;
249246
export { _class as class };
250247
*/
251248
const newNodes = [
252-
makeConst(/*modifiers*/ undefined, rename, right),
249+
makeConst(/*modifiers*/ undefined, rename, assignment.right),
253250
makeExportDeclaration([createExportSpecifier(rename, text)]),
254251
];
255-
changes.replaceNodeWithNodes(sourceFile, statement, newNodes);
252+
changes.replaceNodeWithNodes(sourceFile, assignment.parent, newNodes);
256253
}
257254
else {
258-
changes.replaceNode(sourceFile, statement, convertExportsDotXEquals(text, right));
255+
convertExportsPropertyAssignment(assignment, sourceFile, changes);
259256
}
260257
}
261258

@@ -303,7 +300,27 @@ namespace ts.codefix {
303300
return makeExportDeclaration([createExportSpecifier(/*propertyName*/ undefined, "default")], moduleSpecifier);
304301
}
305302

306-
function convertExportsDotXEquals(name: string | undefined, exported: Expression): Statement {
303+
function convertExportsPropertyAssignment({ left, right, parent }: BinaryExpression & { left: PropertyAccessExpression }, sourceFile: SourceFile, changes: textChanges.ChangeTracker): void {
304+
const name = left.name.text;
305+
if ((isFunctionExpression(right) || isArrowFunction(right) || isClassExpression(right)) && (!right.name || right.name.text === name)) {
306+
// `exports.f = function() {}` -> `export function f() {}` -- Replace `exports.f = ` with `export `, and insert the name after `function`.
307+
changes.replaceRange(sourceFile, { pos: left.getStart(sourceFile), end: right.getStart(sourceFile) }, createToken(SyntaxKind.ExportKeyword), { suffix: " " });
308+
309+
if (!right.name) changes.insertName(sourceFile, right, name);
310+
311+
const semi = findChildOfKind(parent, SyntaxKind.SemicolonToken, sourceFile);
312+
if (semi) changes.deleteNode(sourceFile, semi, { useNonAdjustedEndPosition: true });
313+
}
314+
else {
315+
// `exports.f = function g() {}` -> `export const f = function g() {}` -- just replace `exports.` with `export const `
316+
changes.replaceNodeRangeWithNodes(sourceFile, left.expression, findChildOfKind(left, SyntaxKind.DotToken, sourceFile)!,
317+
[createToken(SyntaxKind.ExportKeyword), createToken(SyntaxKind.ConstKeyword)],
318+
{ joiner: " ", suffix: " " });
319+
}
320+
}
321+
322+
// TODO: GH#22492 this will cause an error if a change has been made inside the body of the node.
323+
function convertExportsDotXEquals_replaceNode(name: string | undefined, exported: Expression): Statement {
307324
const modifiers = [createToken(SyntaxKind.ExportKeyword)];
308325
switch (exported.kind) {
309326
case SyntaxKind.FunctionExpression: {

src/services/textChanges.ts

+38-5
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,10 @@ namespace ts.textChanges {
100100
preserveLeadingWhitespace?: boolean;
101101
}
102102

103+
export interface ReplaceWithMultipleNodesOptions extends InsertNodeOptions {
104+
readonly joiner?: string;
105+
}
106+
103107
enum ChangeKind {
104108
Remove,
105109
ReplaceWithSingleNode,
@@ -130,7 +134,7 @@ namespace ts.textChanges {
130134
interface ReplaceWithMultipleNodes extends BaseChange {
131135
readonly kind: ChangeKind.ReplaceWithMultipleNodes;
132136
readonly nodes: ReadonlyArray<Node>;
133-
readonly options?: InsertNodeOptions;
137+
readonly options?: ReplaceWithMultipleNodesOptions;
134138
}
135139

136140
interface ChangeText extends BaseChange {
@@ -303,7 +307,7 @@ namespace ts.textChanges {
303307
this.replaceRange(sourceFile, getAdjustedRange(sourceFile, startNode, endNode, options), newNode, options);
304308
}
305309

306-
private replaceRangeWithNodes(sourceFile: SourceFile, range: TextRange, newNodes: ReadonlyArray<Node>, options: InsertNodeOptions = {}) {
310+
private replaceRangeWithNodes(sourceFile: SourceFile, range: TextRange, newNodes: ReadonlyArray<Node>, options: ReplaceWithMultipleNodesOptions & ConfigurableStartEnd = {}) {
307311
this.changes.push({ kind: ChangeKind.ReplaceWithMultipleNodes, sourceFile, range, options, nodes: newNodes });
308312
return this;
309313
}
@@ -312,7 +316,7 @@ namespace ts.textChanges {
312316
return this.replaceRangeWithNodes(sourceFile, getAdjustedRange(sourceFile, oldNode, oldNode, options), newNodes, options);
313317
}
314318

315-
public replaceNodeRangeWithNodes(sourceFile: SourceFile, startNode: Node, endNode: Node, newNodes: ReadonlyArray<Node>, options: ChangeNodeOptions = useNonAdjustedPositions) {
319+
public replaceNodeRangeWithNodes(sourceFile: SourceFile, startNode: Node, endNode: Node, newNodes: ReadonlyArray<Node>, options: ReplaceWithMultipleNodesOptions & ConfigurableStartEnd = useNonAdjustedPositions) {
316320
return this.replaceRangeWithNodes(sourceFile, getAdjustedRange(sourceFile, startNode, endNode, options), newNodes, options);
317321
}
318322

@@ -326,7 +330,7 @@ namespace ts.textChanges {
326330
this.replaceRange(sourceFile, createTextRange(pos), newNode, options);
327331
}
328332

329-
private insertNodesAt(sourceFile: SourceFile, pos: number, newNodes: ReadonlyArray<Node>, options: InsertNodeOptions = {}): void {
333+
private insertNodesAt(sourceFile: SourceFile, pos: number, newNodes: ReadonlyArray<Node>, options: ReplaceWithMultipleNodesOptions = {}): void {
330334
this.changes.push({ kind: ChangeKind.ReplaceWithMultipleNodes, sourceFile, options, nodes: newNodes, range: { pos, end: pos } });
331335
}
332336

@@ -483,6 +487,35 @@ namespace ts.textChanges {
483487
return Debug.failBadSyntaxKind(node); // We haven't handled this kind of node yet -- add it
484488
}
485489

490+
public insertName(sourceFile: SourceFile, node: FunctionExpression | ClassExpression | ArrowFunction, name: string): void {
491+
Debug.assert(!node.name);
492+
if (node.kind === SyntaxKind.ArrowFunction) {
493+
const arrow = findChildOfKind(node, SyntaxKind.EqualsGreaterThanToken, sourceFile)!;
494+
const lparen = findChildOfKind(node, SyntaxKind.OpenParenToken, sourceFile);
495+
if (lparen) {
496+
// `() => {}` --> `function f() {}`
497+
this.insertNodesAt(sourceFile, lparen.getStart(sourceFile), [createToken(SyntaxKind.FunctionKeyword), createIdentifier(name)], { joiner: " " });
498+
this.deleteNode(sourceFile, arrow);
499+
}
500+
else {
501+
// `x => {}` -> `function f(x) {}`
502+
this.insertText(sourceFile, first(node.parameters).getStart(sourceFile), `function ${name}(`);
503+
// Replacing full range of arrow to get rid of the leading space -- replace ` =>` with `)`
504+
this.replaceRange(sourceFile, arrow, createToken(SyntaxKind.CloseParenToken));
505+
}
506+
507+
if (node.body.kind !== SyntaxKind.Block) {
508+
// `() => 0` => `function f() { return 0; }`
509+
this.insertNodesAt(sourceFile, node.body.getStart(sourceFile), [createToken(SyntaxKind.OpenBraceToken), createToken(SyntaxKind.ReturnKeyword)], { joiner: " ", suffix: " " });
510+
this.insertNodesAt(sourceFile, node.body.end, [createToken(SyntaxKind.SemicolonToken), createToken(SyntaxKind.CloseBraceToken)], { joiner: " " });
511+
}
512+
}
513+
else {
514+
const pos = findChildOfKind(node, node.kind === SyntaxKind.FunctionExpression ? SyntaxKind.FunctionKeyword : SyntaxKind.ClassKeyword, sourceFile)!.end;
515+
this.insertNodeAt(sourceFile, pos, createIdentifier(name), { prefix: " " });
516+
}
517+
}
518+
486519
/**
487520
* This function should be used to insert nodes in lists when nodes don't carry separators as the part of the node range,
488521
* i.e. arguments in arguments lists, parameters in parameter lists etc.
@@ -652,7 +685,7 @@ namespace ts.textChanges {
652685
const { options = {}, range: { pos } } = change;
653686
const format = (n: Node) => getFormattedTextOfNode(n, sourceFile, pos, options, newLineCharacter, formatContext, validate);
654687
const text = change.kind === ChangeKind.ReplaceWithMultipleNodes
655-
? change.nodes.map(n => removeSuffix(format(n), newLineCharacter)).join(newLineCharacter)
688+
? change.nodes.map(n => removeSuffix(format(n), newLineCharacter)).join(change.options.joiner || newLineCharacter)
656689
: format(change.node);
657690
// strip initial indentation (spaces or tabs) if text will be inserted in the middle of the line
658691
const noIndent = (options.preserveLeadingWhitespace || options.indentation !== undefined || getLineStartPositionForPosition(pos, sourceFile) === pos) ? text : text.replace(/^\s+/, "");

tests/cases/fourslash/refactorConvertToEs6Module_export_alias.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,6 @@
1010
verify.codeFix({
1111
description: "Convert to ES6 module",
1212
newFileContent: `
13-
export function f() { }
13+
export function f() {}
1414
`,
1515
});

tests/cases/fourslash/refactorConvertToEs6Module_export_named.ts

+11-4
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@
66
////[|exports.f = function() {}|];
77
////exports.C = class {};
88
////exports.x = 0;
9+
////exports.a1 = () => {};
10+
////exports.a2 = () => 0;
11+
////exports.a3 = x => { x; };
12+
////exports.a4 = x => x;
913

1014
verify.getSuggestionDiagnostics([{
1115
message: "File is a CommonJS module; it may be converted to an ES6 module.",
@@ -15,8 +19,11 @@ verify.getSuggestionDiagnostics([{
1519
verify.codeFix({
1620
description: "Convert to ES6 module",
1721
newFileContent:
18-
`export function f() { }
19-
export class C {
20-
}
21-
export const x = 0;`,
22+
`export function f() {}
23+
export class C {}
24+
export const x = 0;
25+
export function a1() {}
26+
export function a2() { return 0; }
27+
export function a3(x) { x; }
28+
export function a4(x) { return x; }`,
2229
});
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
/// <reference path='fourslash.ts' />
2+
3+
// @allowJs: true
4+
5+
// @Filename: /a.js
6+
////exports.C = class E { static instance = new E(); }
7+
////exports.D = class D { static instance = new D(); }
8+
9+
verify.codeFix({
10+
description: "Convert to ES6 module",
11+
newFileContent:
12+
`export const C = class E { static instance = new E(); }
13+
export class D { static instance = new D(); }`,
14+
});

tests/cases/fourslash/refactorConvertToEs6Module_export_namedFunctionExpression.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,6 @@
99
verify.codeFix({
1010
description: "Convert to ES6 module",
1111
newFileContent:
12-
`export const f = function g() { g(); };
12+
`export const f = function g() { g(); }
1313
export function h() { h(); }`,
1414
});

tests/cases/fourslash/refactorConvertToEs6Module_export_referenced.ts

+4-3
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
////
1313
////exports.z = 2;
1414
////exports.f = function(z) {
15-
//// z;
15+
//// exports.z; z;
1616
////}
1717

1818
// TODO: GH#22492 Should be a able access `exports.z` inside `exports.f`
@@ -28,8 +28,9 @@ const _y = y;
2828
export { _y as y };
2929
_y;
3030
31-
export const z = 2;
31+
const _z = 2;
32+
export { _z as z };
3233
export function f(z) {
33-
z;
34+
_z; z;
3435
}`,
3536
});

tests/cases/fourslash/refactorConvertToEs6Module_expressionToDeclaration.ts

+1-3
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,5 @@ verify.codeFix({
1010
description: "Convert to ES6 module",
1111
newFileContent:
1212
`export async function* f(p) { p; }
13-
export class C extends D {
14-
m() { }
15-
}`,
13+
export class C extends D { m() {} }`,
1614
});

0 commit comments

Comments
 (0)