Skip to content

TS Server format line fixes #2258

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.

Sign up for GitHub

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 6 commits into from
Mar 9, 2015
Merged

TS Server format line fixes #2258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8bef6d7
Added additional cases for format on enter. These cases fix bugs in the
Mar 7, 2015
9a45160
Changed TypeScript server logging to use an environment variable TSS_LOG
Mar 8, 2015
865802a
Added new logger methods to test harness.
Mar 8, 2015
360e478
Addressed comments on code style and organization.
Mar 8, 2015
04320e4
Renames and comments as requested by feedback.
Mar 9, 2015
37c6f69
Hold at most 4 snapshots per file in the server. This is about 1K pe…
Mar 9, 2015
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
10 changes: 10 additions & 0 deletions src/harness/harnessLanguageService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,7 @@ module Harness.LanguageService {
this.writeMessage(message);
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary newline.

readFile(fileName: string): string {
if (fileName.indexOf(Harness.Compiler.defaultLibFileName) >= 0) {
fileName = Harness.Compiler.defaultLibFileName;
Expand Down Expand Up @@ -526,6 +527,15 @@ module Harness.LanguageService {
msg(message: string) {
return this.host.log(message);
}

loggingEnabled() {
return true;
}

isVerbose() {
return false;
}


endGroup(): void {
}
Expand Down
10 changes: 10 additions & 0 deletions src/server/editorServices.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
module ts.server {
export interface Logger {
close(): void;
isVerbose(): boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

these will require changes on the VS side as well to implement the new APIs. alternativelly you can use one of the methods of the old Logger interface that VS still implements:

        bool information();
        bool debug();
        bool warning();
        bool error();
        bool fatal();

alternatively you can make them optional, and add a comment.

loggingEnabled(): boolean;
perftrc(s: string): void;
info(s: string): void;
startGroup(): void;
Expand Down Expand Up @@ -1071,6 +1073,7 @@ module ts.server {

static changeNumberThreshold = 8;
static changeLengthThreshold = 256;
static maxVersions = 8;

// REVIEW: can optimize by coalescing simple edits
edit(pos: number, deleteLen: number, insertedText?: string) {
Expand Down Expand Up @@ -1131,6 +1134,13 @@ module ts.server {
this.currentVersion = snap.version;
this.versions[snap.version] = snap;
this.changes = [];
if ((this.currentVersion - this.minVersion) >= ScriptVersionCache.maxVersions) {
var oldMin = this.minVersion;
this.minVersion = (this.currentVersion - ScriptVersionCache.maxVersions) + 1;
for (var j = oldMin; j < this.minVersion; j++) {
this.versions[j] = undefined;
}
}
}
return snap;
}
Expand Down
63 changes: 59 additions & 4 deletions src/server/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ module ts.server {
inGroup = false;
firstInGroup = true;

constructor(public logFilename: string) {
constructor(public logFilename: string, public level: string) {
Copy link
Member

Choose a reason for hiding this comment

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

level is too vague; do you mean logging level? I'd prefer this be an enum if there are known states.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

}

static padStringRight(str: string, padding: string) {
Expand Down Expand Up @@ -51,9 +51,20 @@ module ts.server {
this.firstInGroup = true;
}

loggingEnabled() {
return !!this.logFilename;
}

isVerbose() {
return this.loggingEnabled() && (this.level == "verbose");
}


msg(s: string, type = "Err") {
if (this.fd < 0) {
this.fd = fs.openSync(this.logFilename, "w");
if (this.logFilename) {
this.fd = fs.openSync(this.logFilename, "w");
}
}
if (this.fd >= 0) {
s = s + "\n";
Expand Down Expand Up @@ -173,17 +184,61 @@ module ts.server {
});

rl.on('close',() => {
this.projectService.closeLog();
this.projectService.log("Exiting...");
this.projectService.closeLog();
process.exit(0);
});
}
}

interface LogOptions {
file?: string;
detailLevel?: string;
}

function parseLoggingEnvironmentString(logEnvStr: string): LogOptions {
var logEnv: LogOptions = {};
var args = logEnvStr.split(' ');
for (var i = 0, len = args.length; i < (len - 1); i += 2) {
var option = args[i];
var value = args[i + 1];
Copy link
Member

Choose a reason for hiding this comment

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

I think you mean to be incrementing i by 2 each time, and need to check your bounds appropriately (i.e. i < len - 1).

if (option && value) {
switch (option) {
case "-file":
logEnv.file = value;
break;
case "-level":
logEnv.detailLevel = value;
break;
}
}
}
return logEnv;
}

// TSS_LOG "{ level: "normal | verbose | terse", file?: string}"
function createLoggerFromEnv() {
var fileName: string = undefined;
var detailLevel = "normal";
var logEnvStr = process.env["TSS_LOG"];
if (logEnvStr) {
var logEnv = parseLoggingEnvironmentString(logEnvStr);
if (logEnv.file) {
fileName = logEnv.file;
}
else {
fileName = __dirname + "/.log" + process.pid.toString();
Copy link
Member

Choose a reason for hiding this comment

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

Use path.join to join paths; you can also avoid the toString() call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this tie us tightly to node? Can we expose something appropriate from Sys as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Call to toString seems unnecessary.

}
if (logEnv.detailLevel) {
detailLevel = logEnv.detailLevel;
}
}
return new Logger(fileName, detailLevel);
}
// This places log file in the directory containing editorServices.js
// TODO: check that this location is writable
var logger = new Logger(__dirname + "/.log" + process.pid.toString());

var logger = createLoggerFromEnv();

// REVIEW: for now this implementation uses polling.
// The advantage of polling is that it works reliably
Expand Down
75 changes: 61 additions & 14 deletions src/server/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@ module ts.server {

send(msg: NodeJS._debugger.Message) {
var json = JSON.stringify(msg);
if (this.logger.isVerbose()) {
this.logger.info(msg.type + ": " + json);
}
this.sendLineToClient('Content-Length: ' + (1 + Buffer.byteLength(json, 'utf8')) +
'\r\n\r\n' + json);
}
Expand Down Expand Up @@ -461,19 +464,49 @@ module ts.server {
var compilerService = project.compilerService;
var position = compilerService.host.lineColToPosition(file, line, col);
var edits = compilerService.languageService.getFormattingEditsAfterKeystroke(file, position, key,
compilerService.formatCodeOptions);
compilerService.formatCodeOptions);
// Check whether we should auto-indent. This will be when
// the position is on a line containing only whitespace.
// This should leave the edits returned from
// getFormattingEditsAfterKeytroke either empty or pertaining
// only to the previous line. If all this is true, then
// add edits necessary to properly indent the current line.
if ((key == "\n") && ((!edits) || (edits.length == 0) || allEditsBeforePos(edits, position))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we break these up into separate checks. This is fairly confusing to read.

// TODO: get these options from host
var editorOptions: ts.EditorOptions = {
IndentSize: 4,
TabSize: 4,
NewLineCharacter: "\n",
ConvertTabsToSpaces: true,
};
var indentPosition = compilerService.languageService.getIndentationAtPosition(file, position, editorOptions);
var spaces = generateSpaces(indentPosition);
if (indentPosition > 0) {
edits.push({ span: ts.createTextSpanFromBounds(position, position), newText: spaces });
var scriptInfo = compilerService.host.getScriptInfo(file);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we comment what these lines are doing? It's not at all clear to me.

if (scriptInfo) {
var lineInfo = scriptInfo.getLineInfo(line);
if (lineInfo && (lineInfo.leaf) && (lineInfo.leaf.text)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Parens around subexpressions aren't necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you only need to do the below if you have a leaf for this line? What would it mean if you didn't?

var lineText = lineInfo.leaf.text;
if (lineText.search("\\S") < 0) {
// TODO: get these options from host
var editorOptions: ts.EditorOptions = {
IndentSize: 4,
TabSize: 4,
NewLineCharacter: "\n",
ConvertTabsToSpaces: true,
};
var indentPosition =
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really understanding why 'getFormattingEditsAfterKeystroke' needs to know anything about an indent position.

compilerService.languageService.getIndentationAtPosition(file, position, editorOptions);
for (var i = 0, len = lineText.length; i < len; i++) {
if (lineText.charAt(i) == " ") {
indentPosition--;
}
else {
break;
}
}
if (indentPosition > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrmm.. This seems suspect. Why munge with the formatting edits the LS already produced? If the LS edits are correct, why touch them? If they're not correct, why not have the LS itself produce them properly. Otherwise, won't this affects hosts who call into the LS directly?

var spaces = generateSpaces(indentPosition);
edits.push({ span: ts.createTextSpanFromBounds(position, position), newText: spaces });
}
else if (indentPosition < 0) {
edits.push({
span: ts.createTextSpanFromBounds(position, position - indentPosition),
newText: ""
});
}
}
}
}
}

Expand All @@ -491,7 +524,7 @@ module ts.server {
};
});
}

getCompletions(line: number, col: number, prefix: string, fileName: string): protocol.CompletionEntry[] {
if (!prefix) {
prefix = "";
Expand Down Expand Up @@ -693,6 +726,10 @@ module ts.server {
}

onMessage(message: string) {
if (this.logger.isVerbose()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a 'logIfNecessary' helper. Tihs pattern is repeated in several places it seems.

this.logger.info("request: " + message);
var start = process.hrtime();
}
try {
var request = <protocol.Request>JSON.parse(message);
var response: any;
Expand Down Expand Up @@ -798,13 +835,23 @@ module ts.server {
}
}

if (this.logger.isVerbose()) {
var elapsed = process.hrtime(start);
var seconds = elapsed[0]
var nanoseconds = elapsed[1];
var elapsedMs = ((1e9 * seconds) + nanoseconds)/1000000.0;
var leader = "Elapsed time (in milliseconds)";
if (!responseRequired) {
leader = "Async elapsed time (in milliseconds)";
Copy link
Member

Choose a reason for hiding this comment

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

Factor this out into a conditional

}
this.logger.msg(leader + ": " + elapsedMs.toFixed(4).toString(), "Perf");
}
if (response) {
this.output(response, request.command, request.seq);
}
else if (responseRequired) {
this.output(undefined, request.command, request.seq, "No content available.");
}

} catch (err) {
if (err instanceof OperationCanceledException) {
// Handle cancellation exceptions
Expand Down