Skip to content

In insertNodeAfter, handle file with no trailing newline #23814

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.

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
1 commit merged into from
May 3, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2652,10 +2652,10 @@ Actual: ${stringify(fullActual)}`);
public verifyImportFixAtPosition(expectedTextArray: string[], errorCode: number | undefined, preferences: ts.UserPreferences | undefined) {
const { fileName } = this.activeFile;
const ranges = this.getRanges().filter(r => r.fileName === fileName);
if (ranges.length !== 1) {
if (ranges.length > 1) {
this.raiseError("Exactly one range should be specified in the testfile.");
}
const range = ts.first(ranges);
const range = ts.firstOrUndefined(ranges);

const codeFixes = this.getCodeFixes(fileName, errorCode, preferences).filter(f => f.fixId === undefined); // TODO: GH#20315 filter out those that use the import fix ID;

Expand All @@ -2674,7 +2674,7 @@ Actual: ${stringify(fullActual)}`);
const change = ts.first(codeFix.changes);
ts.Debug.assert(change.fileName === fileName);
this.applyEdits(change.fileName, change.textChanges, /*isFormattingEdit*/ false);
const text = this.rangeText(range);
const text = range ? this.rangeText(range) : this.getFileContent(this.activeFile.fileName);
actualTextArray.push(text);
scriptInfo.updateContent(originalContent);
}
Expand Down
10 changes: 7 additions & 3 deletions src/services/textChanges.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,10 +177,10 @@ namespace ts.textChanges {
}

function getAdjustedEndPosition(sourceFile: SourceFile, node: Node, options: ConfigurableEnd) {
const { end } = node;
if (options.useNonAdjustedEndPosition || isExpression(node)) {
return node.getEnd();
return end;
}
const end = node.getEnd();
const newEnd = skipTrivia(sourceFile.text, end, /*stopAfterLineBreak*/ true);
return newEnd !== end && isLineBreak(sourceFile.text.charCodeAt(newEnd - 1))
? newEnd
Expand Down Expand Up @@ -466,7 +466,11 @@ namespace ts.textChanges {
}
}
const endPosition = getAdjustedEndPosition(sourceFile, after, {});
return this.replaceRange(sourceFile, createTextRange(endPosition), newNode, this.getInsertNodeAfterOptions(after));
const options = this.getInsertNodeAfterOptions(after);
return this.replaceRange(sourceFile, createTextRange(endPosition), newNode, {
...options,
prefix: after.end === sourceFile.end && isStatement(after) ? (options.prefix ? `\n${options.prefix}` : "\n") : options.prefix,
});
}

private getInsertNodeAfterOptions(node: Node): InsertNodeOptions {
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/textChanges/insertNodeAfter2.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ namespace M {
// comment 6
var a = 4; // comment 7
}

public class class1 implements interface1
{
property1: boolean;
Expand Down
1 change: 1 addition & 0 deletions tests/cases/fourslash/codeFixAddMissingMember5.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ verify.codeFix({
()=>{ this.foo === 10 };
}
}

C.foo = undefined;
`
});
1 change: 1 addition & 0 deletions tests/cases/fourslash/codeFixAddMissingMember7.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ verify.codeFix({
newFileContent: `class C {
static p = ()=>{ this.foo === 10 };
}

C.foo = undefined;
`
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
verify.codeFix({
description: "Convert function to an ES2015 class",
newFileContent:
`/** Doc */
`
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem great. What is the comment being inserted after that is resulting in the blank line?

Copy link
Author

Choose a reason for hiding this comment

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

The way convertFunctionToEs6Class works, we are deleting the function and adding a class node after it. In textChanges we don't know that the function is going to be deleted, so we add a newline to separate the old function and the new class. Added an issue at #23871

/** Doc */
class C {
constructor() { this.x = 0; }
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/// <reference path="fourslash.ts" />

// @Filename: /a.ts
////export const foo = 0;

// @Filename: /b.ts
////export const bar = 0;

// @Filename: /c.ts
////foo;
////import { bar } from "./b";

goTo.file("/c.ts");
verify.importFixAtPosition([
`foo;
import { bar } from "./b";
import { foo } from "./a";
`,
]);
7 changes: 5 additions & 2 deletions tests/cases/fourslash/importNameCodeFix_jsExtension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@
////import * as g from "global"; // Global imports skipped
////import { a } from "./a.js";
////import { a as a2 } from "./a"; // Ignored, only the first relative import is considered
////[|b;|]
////b;

goTo.file("/c.ts");
verify.importFixAtPosition([
`import { b } from "./b.js";
`import * as g from "global"; // Global imports skipped
import { a } from "./a.js";
import { a as a2 } from "./a"; // Ignored, only the first relative import is considered
import { b } from "./b.js";
b;`,
]);
9 changes: 5 additions & 4 deletions tests/cases/fourslash/importNameCodeFix_jsx.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@

// @Filename: /c.tsx
////import { React } from "react";
////[|<Foo />;|]
////<Foo />;

// @Filename: /d.tsx
////[|import { Foo } from "./Foo";
////<Foo />;|]
////import { Foo } from "./Foo";
////<Foo />;

// Tests that we don't crash at non-identifier location.
goTo.file("/a.tsx");
Expand All @@ -26,7 +26,8 @@ verify.importFixAtPosition([]);
// When constructor is missing, provide fix for that
goTo.file("/c.tsx");
verify.importFixAtPosition([
`import { Foo } from "./Foo";
`import { React } from "react";
import { Foo } from "./Foo";
<Foo />;`]);

// When JSX namespace is missing, provide fix for that
Expand Down