-
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
Fix(29118): tsconfig.extends as array #50403
Conversation
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.
All these you can either add assert that type is not "listOrElement"
https://github.com/microsoft/TypeScript/blob/main/src/compiler/builder.ts#L997
https://github.com/microsoft/TypeScript/blob/main/src/compiler/commandLineParser.ts#L2491
https://github.com/microsoft/TypeScript/blob/main/src/compiler/commandLineParser.ts#L2664
https://github.com/microsoft/TypeScript/blob/main/src/executeCommandLine/executeCommandLine.ts#L166
https://github.com/microsoft/TypeScript/blob/main/src/executeCommandLine/executeCommandLine.ts#L282
https://github.com/microsoft/TypeScript/blob/main/src/services/getEditsForFileRename.ts#L76
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.
I will look into other comments later
@@ -300,6 +300,14 @@ namespace ts { | |||
// TODO: check infinite loop | |||
possibleValues = getPossibleValues(option.element); | |||
break; | |||
case "listOrElement": | |||
if (option.element.type === "string"){ |
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.
if (option.element.type === "string"){ | |
if (option.element.type === "string") { |
src/compiler/commandLineParser.ts
Outdated
options[opt.name] = validateJsonOptionValue(opt, args[i] || "", errors); | ||
i++; | ||
} | ||
else{ |
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.
else{ | |
else { | |
src/compiler/commandLineParser.ts
Outdated
@@ -2986,34 +3029,52 @@ namespace ts { | |||
if (ownConfig.extendedConfigPath) { | |||
// copy the resolution stack so it is never reused between branches in potential diamond-problem scenarios. | |||
resolutionStack = resolutionStack.concat([resolvedPath]); | |||
const extendedConfig = getExtendedConfig(sourceFile, ownConfig.extendedConfigPath, host, resolutionStack, errors, extendedConfigCache); | |||
const result: ExtendsResult = { options:{} }; | |||
if(isString(ownConfig.extendedConfigPath)){ |
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.
Spaces throughout
src/compiler/commandLineParser.ts
Outdated
i++; | ||
} | ||
} | ||
Debug.fail(); |
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.
Debug.fail(); | |
Debug.fail("listOrElement not supported here"); | |
src/compiler/commandLineParser.ts
Outdated
@@ -2513,6 +2529,7 @@ function serializeOptionBaseObject( | |||
const value = options[name] as CompilerOptionsValue; | |||
const optionDefinition = optionsNameMap.get(name.toLowerCase()); | |||
if (optionDefinition) { | |||
Debug.assert(optionDefinition.type !== "listOrElement"); |
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.
Please fix formatting for some of the changes that has been indented by one extra tab
Example: // @Filename: /tsconfig1.json
{
"compilerOptions": {
"strictNullChecks": true
}
}
// @Filename: /tsconfig2.json
{
"compilerOptions": {
"noImplicitAny": true
}
}
// @Filename: /tsconfig.json
{
"extends": ["./tsconfig1.json", "./tsconfig2.json"]
"files": ["./index.ts"]
}
// @Filename: /index.ts
function f(x) {} // noImplicitAny error
let y: string;
y.toLowerCase(); // strictNullChecks error |
@andrewbranch the compiler tests are hard to add with tsconfig. https://github.com/microsoft/TypeScript/pull/50403/files#diff-0556827a3985d4ffd3ed2e51ac63c9d86ba98ab41dca5c7711fb2750188cbe8a shows the test case where multiple tsconfigs are working together ? https://github.com/microsoft/TypeScript/pull/50403/files#diff-97bc6a439e0f02498deab0f7c919e4e7a2f06c6b4832fd2fc7b042f56b141ff9R111 baseline. While said that we could add test in non watch mode as well. |
I saw that baseline, but I couldn’t find any indication in the baseline that the options in the extended configs were relevant. I was looking for a There are plenty of compiler tests with tsconfig files, but I don’t know if there are subtle gotchas with them. |
@navya9singh you probably want to add test like https://github.com/microsoft/TypeScript/blob/main/src/testRunner/unittests/tsc/forceConsistentCasingInFileNames.ts with the contents of test files as shown by @andrewbranch for better clarity |
@typescript-bot pack this |
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at bb0ae2f. You can monitor the build here. |
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
It looks like some changes landed in |
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.
Once the build is green, looks good to merge.
Very cool |
Just a heads up if you're sharing a The In my case, it was also causing |
Fixes #29118