-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Configuration Inheritance #9941
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1721,6 +1721,8 @@ namespace ts { | |
* @param path The path to test. | ||
*/ | ||
fileExists(path: string): boolean; | ||
|
||
readFile(path: string): string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need to document this as an API breaking change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alexeagle, @chuckjaz, @basarat, @adidahiya, @ivogabe, @jbrantly, and @chancancode this is a breaking change in the API, it adds a requirement for a new method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been just passing in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❤️ @blakeembrey (ts-node / tsconfig) @cartant / @smrq (tsify) @johnnyreilly (also on ts-loader) @sebastian-lenz (typedoc) Sorry for the mention, thought I'd let you know. Feel free to ignore 💟 🌹 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This breaks us in at least one place: What is the timing of this change? Will it be part of 2.0? If so this will be a challenge for us. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chuckjaz It looks like you have a readfile method available, you just dont pass it in as part of the host, so it looks easy to update for future versions. Actually, is there a reason you capture the methods on a new object, rather than just passing in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
no, this is a TS 2.1 feature. but once checked in it will start showing up in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mhegazy If it is part of 2.1 on @weswigham This can be better but we do this, in general, to make testing easier. |
||
} | ||
|
||
export interface WriteFileCallback { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,187 @@ | ||
/// <reference path="..\harness.ts" /> | ||
/// <reference path="..\virtualFileSystem.ts" /> | ||
|
||
namespace ts { | ||
const testContents = { | ||
"/dev/tsconfig.json": `{ | ||
"extends": "./configs/base", | ||
"files": [ | ||
"main.ts", | ||
"supplemental.ts" | ||
] | ||
}`, | ||
"/dev/tsconfig.nostrictnull.json": `{ | ||
"extends": "./tsconfig", | ||
"compilerOptions": { | ||
"strictNullChecks": false | ||
} | ||
}`, | ||
"/dev/configs/base.json": `{ | ||
"compilerOptions": { | ||
"allowJs": true, | ||
"noImplicitAny": true, | ||
"strictNullChecks": true | ||
} | ||
}`, | ||
"/dev/configs/tests.json": `{ | ||
"compilerOptions": { | ||
"preserveConstEnums": true, | ||
"removeComments": false, | ||
"sourceMap": true | ||
}, | ||
"exclude": [ | ||
"../tests/baselines", | ||
"../tests/scenarios" | ||
], | ||
"include": [ | ||
"../tests/**/*.ts" | ||
] | ||
}`, | ||
"/dev/circular.json": `{ | ||
"extends": "./circular2", | ||
"compilerOptions": { | ||
"module": "amd" | ||
} | ||
}`, | ||
"/dev/circular2.json": `{ | ||
"extends": "./circular", | ||
"compilerOptions": { | ||
"module": "commonjs" | ||
} | ||
}`, | ||
"/dev/missing.json": `{ | ||
"extends": "./missing2", | ||
"compilerOptions": { | ||
"types": [] | ||
} | ||
}`, | ||
"/dev/failure.json": `{ | ||
"extends": "./failure2.json", | ||
"compilerOptions": { | ||
"typeRoots": [] | ||
} | ||
}`, | ||
"/dev/failure2.json": `{ | ||
"excludes": ["*.js"] | ||
}`, | ||
"/dev/configs/first.json": `{ | ||
"extends": "./base", | ||
"compilerOptions": { | ||
"module": "commonjs" | ||
}, | ||
"files": ["../main.ts"] | ||
}`, | ||
"/dev/configs/second.json": `{ | ||
"extends": "./base", | ||
"compilerOptions": { | ||
"module": "amd" | ||
}, | ||
"include": ["../supplemental.*"] | ||
}`, | ||
"/dev/extends.json": `{ "extends": 42 }`, | ||
"/dev/extends2.json": `{ "extends": "configs/base" }`, | ||
"/dev/main.ts": "", | ||
"/dev/supplemental.ts": "", | ||
"/dev/tests/unit/spec.ts": "", | ||
"/dev/tests/utils.ts": "", | ||
"/dev/tests/scenarios/first.json": "", | ||
"/dev/tests/baselines/first/output.ts": "" | ||
}; | ||
|
||
const caseInsensitiveBasePath = "c:/dev/"; | ||
const caseInsensitiveHost = new Utils.MockParseConfigHost(caseInsensitiveBasePath, /*useCaseSensitiveFileNames*/ false, mapObject(testContents, (key, content) => [`c:${key}`, content])); | ||
|
||
const caseSensitiveBasePath = "/dev/"; | ||
const caseSensitiveHost = new Utils.MockParseConfigHost(caseSensitiveBasePath, /*useCaseSensitiveFileNames*/ true, testContents); | ||
|
||
function verifyDiagnostics(actual: Diagnostic[], expected: {code: number, category: DiagnosticCategory, messageText: string}[]) { | ||
assert.isTrue(expected.length === actual.length, `Expected error: ${JSON.stringify(expected)}. Actual error: ${JSON.stringify(actual)}.`); | ||
for (let i = 0; i < actual.length; i++) { | ||
const actualError = actual[i]; | ||
const expectedError = expected[i]; | ||
assert.equal(actualError.code, expectedError.code, "Error code mismatch"); | ||
assert.equal(actualError.category, expectedError.category, "Category mismatch"); | ||
assert.equal(flattenDiagnosticMessageText(actualError.messageText, "\n"), expectedError.messageText); | ||
} | ||
} | ||
|
||
describe("Configuration Extension", () => { | ||
forEach<[string, string, Utils.MockParseConfigHost], void>([ | ||
["under a case insensitive host", caseInsensitiveBasePath, caseInsensitiveHost], | ||
["under a case sensitive host", caseSensitiveBasePath, caseSensitiveHost] | ||
], ([testName, basePath, host]) => { | ||
function testSuccess(name: string, entry: string, expected: CompilerOptions, expectedFiles: string[]) { | ||
it(name, () => { | ||
const {config, error} = ts.readConfigFile(entry, name => host.readFile(name)); | ||
assert(config && !error, flattenDiagnosticMessageText(error && error.messageText, "\n")); | ||
const parsed = ts.parseJsonConfigFileContent(config, host, basePath, {}, entry); | ||
assert(!parsed.errors.length, flattenDiagnosticMessageText(parsed.errors[0] && parsed.errors[0].messageText, "\n")); | ||
expected.configFilePath = entry; | ||
assert.deepEqual(parsed.options, expected); | ||
assert.deepEqual(parsed.fileNames, expectedFiles); | ||
}); | ||
} | ||
|
||
function testFailure(name: string, entry: string, expectedDiagnostics: {code: number, category: DiagnosticCategory, messageText: string}[]) { | ||
it(name, () => { | ||
const {config, error} = ts.readConfigFile(entry, name => host.readFile(name)); | ||
assert(config && !error, flattenDiagnosticMessageText(error && error.messageText, "\n")); | ||
const parsed = ts.parseJsonConfigFileContent(config, host, basePath, {}, entry); | ||
verifyDiagnostics(parsed.errors, expectedDiagnostics); | ||
}); | ||
} | ||
|
||
describe(testName, () => { | ||
testSuccess("can resolve an extension with a base extension", "tsconfig.json", { | ||
allowJs: true, | ||
noImplicitAny: true, | ||
strictNullChecks: true, | ||
}, [ | ||
combinePaths(basePath, "main.ts"), | ||
combinePaths(basePath, "supplemental.ts"), | ||
]); | ||
|
||
testSuccess("can resolve an extension with a base extension that overrides options", "tsconfig.nostrictnull.json", { | ||
allowJs: true, | ||
noImplicitAny: true, | ||
strictNullChecks: false, | ||
}, [ | ||
combinePaths(basePath, "main.ts"), | ||
combinePaths(basePath, "supplemental.ts"), | ||
]); | ||
|
||
testFailure("can report errors on circular imports", "circular.json", [ | ||
{ | ||
code: 18000, | ||
category: DiagnosticCategory.Error, | ||
messageText: `Circularity detected while resolving configuration: ${[combinePaths(basePath, "circular.json"), combinePaths(basePath, "circular2.json"), combinePaths(basePath, "circular.json")].join(" -> ")}` | ||
} | ||
]); | ||
|
||
testFailure("can report missing configurations", "missing.json", [{ | ||
code: 6096, | ||
category: DiagnosticCategory.Message, | ||
messageText: `File './missing2' does not exist.` | ||
}]); | ||
|
||
testFailure("can report errors in extended configs", "failure.json", [{ | ||
code: 6114, | ||
category: DiagnosticCategory.Error, | ||
messageText: `Unknown option 'excludes'. Did you mean 'exclude'?` | ||
}]); | ||
|
||
testFailure("can error when 'extends' is not a string", "extends.json", [{ | ||
code: 5024, | ||
category: DiagnosticCategory.Error, | ||
messageText: `Compiler option 'extends' requires a value of type string.` | ||
}]); | ||
|
||
testFailure("can error when 'extends' is neither relative nor rooted.", "extends2.json", [{ | ||
code: 18001, | ||
category: DiagnosticCategory.Error, | ||
messageText: `The path in an 'extends' options must be relative or rooted.` | ||
}]); | ||
}); | ||
}); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kinda strange that you force them to write
.\
but not the extension. i would say either be pedantic all the way or not. so i would just try to load the file, if it exists fine, if not error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reasoning for forbidding modules identifier like paths is, as described in the original issue, to reserve the form for potentially loading configs from
node_modules
in the future (to align with module resolution) (this way adding that isn't a breaking change) (eslint does this). If you'd like, you can spec that behavior out; but without that I think it'd be better to just reserve the form with an error for now.