Skip to content

Commit

Permalink
fix(50654): "Move to a new file" breaks the declaration of referenced…
Browse files Browse the repository at this point in the history
… variable (#50681)

* fix(50654): remove entire import require call instead of the name

* handle require imports in ts files
  • Loading branch information
a-tarasyuk authored Oct 26, 2022
1 parent 170a17f commit 8b1ecdb
Show file tree
Hide file tree
Showing 4 changed files with 109 additions and 3 deletions.
18 changes: 15 additions & 3 deletions src/services/refactors/moveToNewFile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ namespace ts.refactor {
) {
const checker = program.getTypeChecker();
const prologueDirectives = takeWhile(oldFile.statements, isPrologueDirective);
if (!oldFile.externalModuleIndicator && !oldFile.commonJsModuleIndicator) {
if (oldFile.externalModuleIndicator === undefined && oldFile.commonJsModuleIndicator === undefined && usage.oldImportsNeededByNewFile.size() === 0) {
deleteMovedStatements(oldFile, toMove.ranges, changes);
return [...prologueDirectives, ...toMove.all];
}
Expand Down Expand Up @@ -402,7 +402,13 @@ namespace ts.refactor {
switch (name.kind) {
case SyntaxKind.Identifier:
if (isUnused(name)) {
changes.delete(sourceFile, name);
if (varDecl.initializer && isRequireCall(varDecl.initializer, /*requireStringLiteralLikeArgument*/ true)) {
changes.delete(sourceFile,
isVariableDeclarationList(varDecl.parent) && length(varDecl.parent.declarations) === 1 ? varDecl.parent.parent : varDecl);
}
else {
changes.delete(sourceFile, name);
}
}
break;
case SyntaxKind.ArrayBindingPattern:
Expand Down Expand Up @@ -641,10 +647,12 @@ namespace ts.refactor {
}

interface ReadonlySymbolSet {
size(): number;
has(symbol: Symbol): boolean;
forEach(cb: (symbol: Symbol) => void): void;
forEachEntry<T>(cb: (symbol: Symbol) => T | undefined): T | undefined;
}

class SymbolSet implements ReadonlySymbolSet {
private map = new Map<string, Symbol>();
add(symbol: Symbol): void {
Expand All @@ -667,6 +675,9 @@ namespace ts.refactor {
copyEntries(this.map, clone.map);
return clone;
}
size() {
return this.map.size;
}
}

type TopLevelExpressionStatement = ExpressionStatement & { expression: BinaryExpression & { left: PropertyAccessExpression } }; // 'exports.x = ...'
Expand Down Expand Up @@ -775,7 +786,8 @@ namespace ts.refactor {
if (useEs6Exports) {
return !isExpressionStatement(decl) && hasSyntacticModifier(decl, ModifierFlags.Export) || !!(name && sourceFile.symbol.exports?.has(name.escapedText));
}
return getNamesToExportInCommonJS(decl).some(name => sourceFile.symbol.exports!.has(escapeLeadingUnderscores(name)));
return !!sourceFile.symbol && !!sourceFile.symbol.exports &&
getNamesToExportInCommonJS(decl).some(name => sourceFile.symbol.exports!.has(escapeLeadingUnderscores(name)));
}

function addExport(decl: TopLevelDeclarationStatement, useEs6Exports: boolean): readonly Statement[] | undefined {
Expand Down
28 changes: 28 additions & 0 deletions tests/cases/fourslash/moveToNewFile_requireImport1.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/// <reference path="fourslash.ts" />

// @allowJs: true

// @filename: /a.js
////module.exports = 1;

// @filename: /b.js
////var a = require("./a");
////[|function f() {
//// a;
////}|]

verify.noErrors();

verify.moveToNewFile({
newFileContents: {
"/b.js": "",

"/f.js":
`var a = require("./a");
function f() {
a;
}
`,
},
});
33 changes: 33 additions & 0 deletions tests/cases/fourslash/moveToNewFile_requireImport2.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/// <reference path="fourslash.ts" />

// @allowJs: true

// @filename: /a.js
////module.exports = 1;

// @filename: /b.js
////var a = require("./a"),
//// b = require("./a"),
//// c = require("./a");
////[|function f() {
//// b;
////}|]

verify.noErrors();

verify.moveToNewFile({
newFileContents: {
"/b.js":
`var a = require("./a"),
c = require("./a");
`,

"/f.js":
`var b = require("./a");
function f() {
b;
}
`,
},
});
33 changes: 33 additions & 0 deletions tests/cases/fourslash/moveToNewFile_requireImport3.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/// <reference path="fourslash.ts" />

// @module: commonjs
// @moduleResolution: node

// @filename: /node.d.ts
////declare var module: any;
////declare var require: any;

// @filename: /a.ts
////module.exports = 1;

// @filename: /b.ts
////var a = require("./a");
////[|function f() {
//// a;
////}|]

verify.noErrors();

verify.moveToNewFile({
newFileContents: {
"/b.ts": "",

"/f.ts":
`var a = require("./a");
function f() {
a;
}
`,
},
});

0 comments on commit 8b1ecdb

Please sign in to comment.