Skip to content
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

Allow comments in tsconfig.json #5450

Merged
merged 5 commits into from
Nov 2, 2015
Merged
Show file tree
Hide file tree
Changes from 4 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
3 changes: 2 additions & 1 deletion Jakefile.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,8 @@ var harnessSources = harnessCoreSources.concat([
"transpile.ts",
"reuseProgramStructure.ts",
"cachingInServerLSHost.ts",
"moduleResolution.ts"
"moduleResolution.ts",
"tsconfigParsing.ts"
].map(function (f) {
return path.join(unittestsDirectory, f);
})).concat([
Expand Down
54 changes: 53 additions & 1 deletion src/compiler/commandLineParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -405,13 +405,65 @@ namespace ts {
*/
export function parseConfigFileTextToJson(fileName: string, jsonText: string): { config?: any; error?: Diagnostic } {
try {
return { config: /\S/.test(jsonText) ? JSON.parse(jsonText) : {} };
let jsonTextWithoutComments = removeComments(jsonText);
return { config: /\S/.test(jsonTextWithoutComments) ? JSON.parse(jsonTextWithoutComments) : {} };
}
catch (e) {
return { error: createCompilerDiagnostic(Diagnostics.Failed_to_parse_file_0_Colon_1, fileName, e.message) };
}
}


/**
* Remove the comments from a json like text.
* Comments can be single line comments (starting with # or //) or multiline comments using / * * /
*
* This method replace comment content by whitespace rather than completely remove them to keep positions in json parsing error reporting accurate.
*/
function removeComments(jsonText: string): string {
let output = "";
let scanner = createScanner(ScriptTarget.ES5, /* skipTrivia */ false, LanguageVariant.Standard, jsonText);
let token: SyntaxKind;
while ((token = scanner.scan()) !== SyntaxKind.EndOfFileToken) {
switch (token) {
case SyntaxKind.SingleLineCommentTrivia:
case SyntaxKind.MultiLineCommentTrivia:
// replace comments with whitespaces to preserve original characters position
Copy link
Member

Choose a reason for hiding this comment

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

whitespaces -> whitespace
characters position -> character positions

output += replaceWithWhitespaces(scanner.getTokenText());
Copy link
Member

Choose a reason for hiding this comment

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

You could just use getTokenPos to get the start, getTextPos to get the end, and get the comment width by subtracting the two. Then use the same approach with Array(commentWidth + 1).join(" ").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well not for multiline comments because we also want to preserve line positions.
I was thinking of using regex scanner.getTokenText().replace(/\S/g, " ") instead of replaceWithWhitespaces but I didn't because of (maybe not valid) performance concerns with large coments.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I didn't consider lines. That would've been my second suggestion, and I think it should be fine. Large comments aren't a big concern, and this is only done once per compilation anyway.

break;
default:
output += scanner.getTokenText();
break;
}
}
return output;

function replaceWithWhitespaces(commentTokenText: string): string {
let result = "";
let pos = 0;
let start = 0;
while (pos < commentTokenText.length) {
if (isLineBreak(commentTokenText.charCodeAt(pos))) {
let nbCharToReplace = pos - start;
result += nSpaces(nbCharToReplace);
result += commentTokenText.charAt(pos);
pos += 1;
start = pos;
}
else {
pos += 1;
}
}
result += nSpaces(pos - start);
return result;

function nSpaces(n: number): string {
return new Array(n + 1).join(" ");
};
}
}


/**
* Parse the contents of a config file (tsconfig.json).
* @param json The contents of the config file to parse
Expand Down
86 changes: 86 additions & 0 deletions tests/cases/unittests/tsconfigParsing.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/// <reference path="..\..\..\src\harness\harness.ts" />
/// <reference path="..\..\..\src\compiler\commandLineParser.ts" />

module ts {
Copy link
Member

Choose a reason for hiding this comment

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

Use a namespace

describe('parseConfigFileTextToJson', () => {
function assertParseResult(jsonText: string, expectedConfigObject: { config?: any; error?: Diagnostic }) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you switch this file to use 4 spaces per indentation?

let parsed = ts.parseConfigFileTextToJson("/apath/tsconfig.json", jsonText);
assert.equal(JSON.stringify(parsed), JSON.stringify(expectedConfigObject));
}

function assertParseError(jsonText: string) {
let parsed = ts.parseConfigFileTextToJson("/apath/tsconfig.json", jsonText);
assert.isTrue(undefined === parsed.config);
assert.isTrue(undefined !== parsed.error);
}

it("returns empty config for file with only whitespaces", () => {
assertParseResult("", { config : {} });
Copy link
Member

Choose a reason for hiding this comment

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

There's a tab on this line

assertParseResult(" ", { config : {} });
});

it("returns empty config for file with comments only", () => {
assertParseResult("// Comment", { config: {} });
assertParseResult("/* Comment*/", { config: {} });
});

it("returns empty config when config is empty object", () => {
assertParseResult("{}", { config: {} });
});

it("returns config object without comments", () => {
assertParseResult(
`{ // Excluded files
"exclude": [
// Exclude d.ts
"file.d.ts"
]
}`, { config: { exclude: ["file.d.ts"] } });

assertParseResult(
`{
/* Excluded
Files
*/
"exclude": [
/* multiline comments can be in the middle of a line */"file.d.ts"
]
}`, { config: { exclude: ["file.d.ts"] } });
});

it("keeps string content untouched", () => {
assertParseResult(
`{
"exclude": [
"xx//file.d.ts"
]
}`, { config: { exclude: ["xx//file.d.ts"] } });
assertParseResult(
`{
"exclude": [
"xx/*file.d.ts*/"
]
}`, { config: { exclude: ["xx/*file.d.ts*/"] } });
});

it("handles escaped characters in strings correctly", () => {
assertParseResult(
`{
"exclude": [
"xx\\"//files"
]
}`, { config: { exclude: ["xx\"//files"] } });

assertParseResult(
`{
"exclude": [
"xx\\\\" // end of line comment
]
}`, { config: { exclude: ["xx\\"] } });
});

it("returns object with error when json is invalid", () => {
assertParseError("invalid");
});
});
}