Skip to content

Commit

Permalink
moveToNewFile: Don't remove empty named imports (#26265)
Browse files Browse the repository at this point in the history
  • Loading branch information
Andy authored Aug 7, 2018
1 parent 794f3a5 commit 1a05f13
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 17 deletions.
30 changes: 18 additions & 12 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2985,7 +2985,7 @@ Actual: ${stringify(fullActual)}`);
}

public verifyApplicableRefactorAvailableAtMarker(negative: boolean, markerName: string) {
const isAvailable = this.getApplicableRefactors(this.getMarkerByName(markerName).position).length > 0;
const isAvailable = this.getApplicableRefactors(this.getMarkerByName(markerName)).length > 0;
if (negative && isAvailable) {
this.raiseError(`verifyApplicableRefactorAvailableAtMarker failed - expected no refactor at marker ${markerName} but found some.`);
}
Expand All @@ -3002,7 +3002,7 @@ Actual: ${stringify(fullActual)}`);
}

public verifyRefactorAvailable(negative: boolean, name: string, actionName?: string) {
let refactors = this.getApplicableRefactors(this.getSelection());
let refactors = this.getApplicableRefactorsAtSelection();
refactors = refactors.filter(r => r.name === name && (actionName === undefined || r.actions.some(a => a.name === actionName)));
const isAvailable = refactors.length > 0;

Expand All @@ -3022,11 +3022,11 @@ Actual: ${stringify(fullActual)}`);
}

public verifyRefactorsAvailable(names: ReadonlyArray<string>): void {
assert.deepEqual(unique(this.getApplicableRefactors(this.getSelection()), r => r.name), names);
assert.deepEqual(unique(this.getApplicableRefactorsAtSelection(), r => r.name), names);
}

public verifyRefactor({ name, actionName, refactors }: FourSlashInterface.VerifyRefactorOptions) {
const actualRefactors = this.getApplicableRefactors(this.getSelection()).filter(r => r.name === name && r.actions.some(a => a.name === actionName));
const actualRefactors = this.getApplicableRefactorsAtSelection().filter(r => r.name === name && r.actions.some(a => a.name === actionName));
this.assertObjectsEqual(actualRefactors, refactors);
}

Expand All @@ -3047,7 +3047,7 @@ Actual: ${stringify(fullActual)}`);

public applyRefactor({ refactorName, actionName, actionDescription, newContent: newContentWithRenameMarker }: FourSlashInterface.ApplyRefactorOptions) {
const range = this.getSelection();
const refactors = this.getApplicableRefactors(range);
const refactors = this.getApplicableRefactorsAtSelection();
const refactorsWithName = refactors.filter(r => r.name === refactorName);
if (refactorsWithName.length === 0) {
this.raiseError(`The expected refactor: ${refactorName} is not available at the marker location.\nAvailable refactors: ${refactors.map(r => r.name)}`);
Expand Down Expand Up @@ -3125,7 +3125,7 @@ Actual: ${stringify(fullActual)}`);
const action = ts.first(refactor.actions);
assert(action.name === "Move to a new file" && action.description === "Move to a new file");

const editInfo = this.languageService.getEditsForRefactor(this.activeFile.fileName, this.formatCodeSettings, range, refactor.name, action.name, options.preferences || ts.emptyOptions)!;
const editInfo = this.languageService.getEditsForRefactor(range.fileName, this.formatCodeSettings, range, refactor.name, action.name, options.preferences || ts.emptyOptions)!;
this.testNewFileContents(editInfo.edits, options.newFileContents, "move to new file");
}

Expand Down Expand Up @@ -3165,21 +3165,21 @@ Actual: ${stringify(fullActual)}`);
formattingOptions?: ts.FormatCodeSettings) {

formattingOptions = formattingOptions || this.formatCodeSettings;
const markerPos = this.getMarkerByName(markerName).position;
const marker = this.getMarkerByName(markerName);

const applicableRefactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, markerPos, ts.emptyOptions);
const applicableRefactors = this.languageService.getApplicableRefactors(this.activeFile.fileName, marker.position, ts.emptyOptions);
const applicableRefactorToApply = ts.find(applicableRefactors, refactor => refactor.name === refactorNameToApply);

if (!applicableRefactorToApply) {
this.raiseError(`The expected refactor: ${refactorNameToApply} is not available at the marker location.`);
}

const editInfo = this.languageService.getEditsForRefactor(this.activeFile.fileName, formattingOptions, markerPos, refactorNameToApply, actionName, ts.emptyOptions)!;
const editInfo = this.languageService.getEditsForRefactor(marker.fileName, formattingOptions, marker.position, refactorNameToApply, actionName, ts.emptyOptions)!;

for (const edit of editInfo.edits) {
this.applyEdits(edit.fileName, edit.textChanges, /*isFormattingEdit*/ false);
}
const actualContent = this.getFileContent(this.activeFile.fileName);
const actualContent = this.getFileContent(marker.fileName);

if (actualContent !== expectedContent) {
this.raiseError(`verifyFileAfterApplyingRefactors failed:\n${showTextDiff(expectedContent, actualContent)}`);
Expand Down Expand Up @@ -3381,8 +3381,14 @@ Actual: ${stringify(fullActual)}`);
test(renameKeys(newFileContents, key => pathUpdater(key) || key), "with file moved");
}

private getApplicableRefactors(positionOrRange: number | ts.TextRange, preferences = ts.emptyOptions): ReadonlyArray<ts.ApplicableRefactorInfo> {
return this.languageService.getApplicableRefactors(this.activeFile.fileName, positionOrRange, preferences) || ts.emptyArray;
private getApplicableRefactorsAtSelection() {
return this.getApplicableRefactorsWorker(this.getSelection(), this.activeFile.fileName);
}
private getApplicableRefactors(rangeOrMarker: Range | Marker, preferences = ts.emptyOptions): ReadonlyArray<ts.ApplicableRefactorInfo> {
return this.getApplicableRefactorsWorker("position" in rangeOrMarker ? rangeOrMarker.position : rangeOrMarker, rangeOrMarker.fileName, preferences);
}
private getApplicableRefactorsWorker(positionOrRange: number | ts.TextRange, fileName: string, preferences = ts.emptyOptions): ReadonlyArray<ts.ApplicableRefactorInfo> {
return this.languageService.getApplicableRefactors(fileName, positionOrRange, preferences) || ts.emptyArray;
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/services/refactors/moveToNewFile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ namespace ts.refactor {
const { name, namedBindings } = importDecl.importClause;
const defaultUnused = !name || isUnused(name);
const namedBindingsUnused = !namedBindings ||
(namedBindings.kind === SyntaxKind.NamespaceImport ? isUnused(namedBindings.name) : namedBindings.elements.every(e => isUnused(e.name)));
(namedBindings.kind === SyntaxKind.NamespaceImport ? isUnused(namedBindings.name) : namedBindings.elements.length !== 0 && namedBindings.elements.every(e => isUnused(e.name)));
if (defaultUnused && namedBindingsUnused) {
changes.delete(sourceFile, importDecl);
}
Expand Down
10 changes: 6 additions & 4 deletions tests/cases/fourslash/moveToNewFile_updateUses.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@

// @Filename: /user.ts
////import { x, y } from "./a";
////

// TODO: GH#23728 Shouldn't need `////` above
////import { x as x2 } from "./a";
////import { y as y2 } from "./a";
////import {} from "./a";

verify.moveToNewFile({
newFileContents: {
Expand All @@ -22,6 +22,8 @@ verify.moveToNewFile({
"/user.ts":
`import { x } from "./a";
import { y } from "./y";
`,
import { x as x2 } from "./a";
import { y as y2 } from "./y";
import {} from "./a";`,
},
});

0 comments on commit 1a05f13

Please sign in to comment.