Skip to content

do not use ScriptVersionCache for closed files #12777

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
merged 1 commit into from
Dec 9, 2016
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
1 change: 1 addition & 0 deletions Jakefile.js
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,7 @@ var harnessSources = harnessCoreSources.concat([
"convertToBase64.ts",
"transpile.ts",
"reuseProgramStructure.ts",
"textStorage.ts",
"cachingInServerLSHost.ts",
"moduleResolution.ts",
"tsconfigParsing.ts",
Expand Down
71 changes: 71 additions & 0 deletions src/harness/unittests/textStorage.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/// <reference path="../harness.ts" />
/// <reference path="../../server/scriptVersionCache.ts"/>
/// <reference path="./tsserverProjectSystem.ts" />

namespace ts.textStorage {
describe("Text storage", () => {
const f = {
path: "/a/app.ts",
content: `
let x = 1;
let y = 2;
function bar(a: number) {
return a + 1;
}`
};

it("text based storage should be have exactly the same as script version cache", () => {

debugger
const host = ts.projectSystem.createServerHost([f]);

const ts1 = new server.TextStorage(host, server.asNormalizedPath(f.path));
const ts2 = new server.TextStorage(host, server.asNormalizedPath(f.path));

ts1.useScriptVersionCache();
ts2.useText();

const lineMap = computeLineStarts(f.content);

for (let line = 0; line < lineMap.length; line++) {
const start = lineMap[line];
const end = line === lineMap.length - 1 ? f.path.length : lineMap[line + 1];

for (let offset = 0; offset < end - start; offset++) {
const pos1 = ts1.lineOffsetToPosition(line + 1, offset + 1);
const pos2 = ts2.lineOffsetToPosition(line + 1, offset + 1);
assert.isTrue(pos1 === pos2, `lineOffsetToPosition ${line + 1}-${offset + 1}: expected ${pos1} to equal ${pos2}`);
}

const {start: start1, length: length1 } = ts1.lineToTextSpan(line);
const {start: start2, length: length2 } = ts2.lineToTextSpan(line);
assert.isTrue(start1 === start2, `lineToTextSpan ${line}::start:: expected ${start1} to equal ${start2}`);
assert.isTrue(length1 === length2, `lineToTextSpan ${line}::length:: expected ${length1} to equal ${length2}`);
}

for (let pos = 0; pos < f.content.length; pos++) {
const { line: line1, offset: offset1 } = ts1.positionToLineOffset(pos);
const { line: line2, offset: offset2 } = ts2.positionToLineOffset(pos);
assert.isTrue(line1 === line2, `positionToLineOffset ${pos}::line:: expected ${line1} to equal ${line2}`);
assert.isTrue(offset1 === offset2, `positionToLineOffset ${pos}::offset:: expected ${offset1} to equal ${offset2}`);
}
});

it("should switch to script version cache if necessary", () => {
const host = ts.projectSystem.createServerHost([f]);
const ts1 = new server.TextStorage(host, server.asNormalizedPath(f.path));

ts1.getSnapshot();
assert.isTrue(!ts1.hasScriptVersionCache(), "should not have script version cache - 1");

ts1.edit(0, 5, " ");
assert.isTrue(ts1.hasScriptVersionCache(), "have script version cache - 1");

ts1.useText();
assert.isTrue(!ts1.hasScriptVersionCache(), "should not have script version cache - 2");

ts1.getLineInfo(0);
assert.isTrue(ts1.hasScriptVersionCache(), "have script version cache - 2");
})
});
}
15 changes: 7 additions & 8 deletions src/harness/unittests/tsserverProjectSystem.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1574,7 +1574,7 @@ namespace ts.projectSystem {
const project = projectService.externalProjects[0];

const scriptInfo = project.getScriptInfo(file1.path);
const snap = scriptInfo.snap();
const snap = scriptInfo.getSnapshot();
const actualText = snap.getText(0, snap.getLength());
assert.equal(actualText, "", `expected content to be empty string, got "${actualText}"`);

Expand All @@ -1587,7 +1587,7 @@ namespace ts.projectSystem {
projectService.closeClientFile(file1.path);

const scriptInfo2 = project.getScriptInfo(file1.path);
const snap2 = scriptInfo2.snap();
const snap2 = scriptInfo2.getSnapshot();
const actualText2 = snap2.getText(0, snap.getLength());
assert.equal(actualText2, "", `expected content to be empty string, got "${actualText2}"`);
});
Expand Down Expand Up @@ -2285,13 +2285,13 @@ namespace ts.projectSystem {
p.updateGraph();

const scriptInfo = p.getScriptInfo(f.path);
checkSnapLength(scriptInfo.snap(), f.content.length);
checkSnapLength(scriptInfo.getSnapshot(), f.content.length);

// open project and replace its content with empty string
projectService.openClientFile(f.path, "");
checkSnapLength(scriptInfo.snap(), 0);
checkSnapLength(scriptInfo.getSnapshot(), 0);
});
function checkSnapLength(snap: server.LineIndexSnapshot, expectedLength: number) {
function checkSnapLength(snap: IScriptSnapshot, expectedLength: number) {
assert.equal(snap.getLength(), expectedLength, "Incorrect snapshot size");
}
});
Expand Down Expand Up @@ -2444,7 +2444,6 @@ namespace ts.projectSystem {
const cwd = {
path: "/a/c"
};
debugger;
const host = createServerHost([f1, config, node, cwd], { currentDirectory: cwd.path });
const projectService = createProjectService(host);
projectService.openClientFile(f1.path);
Expand Down Expand Up @@ -2715,7 +2714,7 @@ namespace ts.projectSystem {

// verify content
const projectServiice = session.getProjectService();
const snap1 = projectServiice.getScriptInfo(f1.path).snap();
const snap1 = projectServiice.getScriptInfo(f1.path).getSnapshot();
assert.equal(snap1.getText(0, snap1.getLength()), tmp.content, "content should be equal to the content of temp file");

// reload from original file file
Expand All @@ -2727,7 +2726,7 @@ namespace ts.projectSystem {
});

// verify content
const snap2 = projectServiice.getScriptInfo(f1.path).snap();
const snap2 = projectServiice.getScriptInfo(f1.path).getSnapshot();
assert.equal(snap2.getText(0, snap2.getLength()), f1.content, "content should be equal to the content of original file");

});
Expand Down
49 changes: 23 additions & 26 deletions src/server/editorServices.ts
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,7 @@ namespace ts.server {
this.handleDeletedFile(info);
}
else {
if (info && (!info.isOpen)) {
if (info && (!info.isScriptOpen())) {
// file has been changed which might affect the set of referenced files in projects that include
// this file and set of inferred projects
info.reloadFromFile();
Expand All @@ -440,7 +440,7 @@ namespace ts.server {

// TODO: handle isOpen = true case

if (!info.isOpen) {
if (!info.isScriptOpen()) {
this.filenameToScriptInfo.remove(info.path);
this.lastDeletedFile = info;

Expand Down Expand Up @@ -634,10 +634,9 @@ namespace ts.server {
// Closing file should trigger re-reading the file content from disk. This is
// because the user may chose to discard the buffer content before saving
// to the disk, and the server's version of the file can be out of sync.
info.reloadFromFile();
info.close();

removeItemFromSet(this.openFiles, info);
info.isOpen = false;

// collect all projects that should be removed
let projectsToRemove: Project[];
Expand Down Expand Up @@ -989,7 +988,7 @@ namespace ts.server {
}
if (toAdd) {
for (const f of toAdd) {
if (f.isOpen && isRootFileInInferredProject(f)) {
if (f.isScriptOpen() && isRootFileInInferredProject(f)) {
// if file is already root in some inferred project
// - remove the file from that project and delete the project if necessary
const inferredProject = f.containingProjects[0];
Expand Down Expand Up @@ -1089,32 +1088,31 @@ namespace ts.server {
getOrCreateScriptInfoForNormalizedPath(fileName: NormalizedPath, openedByClient: boolean, fileContent?: string, scriptKind?: ScriptKind, hasMixedContent?: boolean) {
let info = this.getScriptInfoForNormalizedPath(fileName);
if (!info) {
let content: string;
if (this.host.fileExists(fileName)) {
// by default pick whatever content was supplied as the argument
// if argument was not given - then for mixed content files assume that its content is empty string
content = fileContent || (hasMixedContent ? "" : this.host.readFile(fileName));
}
if (!content) {
if (openedByClient || this.host.fileExists(fileName)) {
info = new ScriptInfo(this.host, fileName, scriptKind, hasMixedContent);

this.filenameToScriptInfo.set(info.path, info);

if (openedByClient) {
content = "";
if (fileContent === undefined) {
// if file is opened by client and its content is not specified - use file text
fileContent = this.host.readFile(fileName) || "";
}
}
}
if (content !== undefined) {
info = new ScriptInfo(this.host, fileName, content, scriptKind, openedByClient, hasMixedContent);
// do not watch files with mixed content - server doesn't know how to interpret it
this.filenameToScriptInfo.set(info.path, info);
if (!info.isOpen && !hasMixedContent) {
info.setWatcher(this.host.watchFile(fileName, _ => this.onSourceFileChanged(fileName)));
else {
// do not watch files with mixed content - server doesn't know how to interpret it
if (!hasMixedContent) {
info.setWatcher(this.host.watchFile(fileName, _ => this.onSourceFileChanged(fileName)));
}
}
}
}
if (info) {
if (fileContent !== undefined) {
info.reload(fileContent);
if (openedByClient && !info.isScriptOpen()) {
info.open(fileContent);
}
if (openedByClient) {
info.isOpen = true;
else if (fileContent !== undefined) {
info.reload(fileContent);
}
}
return info;
Expand Down Expand Up @@ -1230,7 +1228,6 @@ namespace ts.server {
const info = this.getScriptInfoForNormalizedPath(toNormalizedPath(uncheckedFileName));
if (info) {
this.closeOpenFile(info);
info.isOpen = false;
}
this.printProjects();
}
Expand All @@ -1255,7 +1252,7 @@ namespace ts.server {
if (openFiles) {
for (const file of openFiles) {
const scriptInfo = this.getScriptInfo(file.fileName);
Debug.assert(!scriptInfo || !scriptInfo.isOpen);
Debug.assert(!scriptInfo || !scriptInfo.isScriptOpen());
const normalizedPath = scriptInfo ? scriptInfo.fileName : toNormalizedPath(file.fileName);
this.openClientFileWithNormalizedPath(normalizedPath, file.content, tryConvertScriptKindName(file.scriptKind), file.hasMixedContent);
}
Expand Down
2 changes: 1 addition & 1 deletion src/server/lsHost.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ namespace ts.server {
getScriptSnapshot(filename: string): ts.IScriptSnapshot {
const scriptInfo = this.project.getScriptInfoLSHost(filename);
if (scriptInfo) {
return scriptInfo.snap();
return scriptInfo.getSnapshot();
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/server/project.ts
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ namespace ts.server {

containsFile(filename: NormalizedPath, requireOpen?: boolean) {
const info = this.projectService.getScriptInfoForNormalizedPath(filename);
if (info && (info.isOpen || !requireOpen)) {
if (info && (info.isScriptOpen() || !requireOpen)) {
return this.containsScriptInfo(info);
}
}
Expand Down
Loading