-
Notifications
You must be signed in to change notification settings - Fork 12.9k
tsc --init update #61813
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
tsc --init update #61813
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.
Pull Request Overview
Updates the --init
logic to streamline how tsconfig.json
is generated and simplify related tests and CLI handling. Key changes include:
- Refactored
generateTSConfig
signature and implementation to remove file list parameter and output clean, sectioned JSON. - Updated
writeConfigFile
and test invocations to match newgenerateTSConfig
signature. - Added diagnostic message entries for each new section header.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/compiler/commandLineParser.ts | Replaced old getCompilerOptionsDiffValue and rewritten generateTSConfig with new API. |
src/compiler/executeCommandLine.ts | Updated writeConfigFile to call new generateTSConfig signature and cleaned output. |
src/testRunner/unittests/config/initializeTSConfig.ts | Adjusted test helper to use new generateTSConfig(options, newLine) call. |
src/compiler/diagnosticMessages.json | Inserted new diagnostic entries for section headers used in generated JSON. |
Comments suppressed due to low confidence (1)
src/compiler/commandLineParser.ts:2930
- Consider appending a trailing newline to the generated config for consistency with existing baselines and editor tooling. For example:
return result.join(newLine) + newLine;
return result.join(newLine);
export function generateTSConfig(options: CompilerOptions, fileNames: readonly string[], newLine: string): string { | ||
const compilerOptionsMap = getSerializedCompilerOption(options); | ||
return writeConfigurations(); | ||
export function generateTSConfig(options: CompilerOptions, newLine: string): 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.
I was like "oh no, hope nobody calls this", but no, it's all callers from 5+ years ago (https://github.com/search?q=%2Fts%28+as+%5B%5E%28%5D%2B%5C%29%29%3F%5C.generateTSConfig%5C%28%2F+-path%3Atestrunner&type=code)
tests/baselines/reference/config/initTSConfig/Default initialized TSConfig/tsconfig.json
Outdated
Show resolved
Hide resolved
// Other Outputs | ||
"sourceMap": true, | ||
"declaration": true, | ||
"declarationMap": true, |
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.
probably already discussed when i wasnt here, but declarationMap seems like overkill ? and additional cost?
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'd suggest we just comment it out.
// See also https://aka.ms/tsconfig/module | ||
"module": "nodenext", | ||
"target": "esnext", | ||
"types": [], |
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.
Maybe a silly idea, but should we note the effect that this has?
"files": [ | ||
"file0.st", | ||
"file1.ts", | ||
"file2.ts" | ||
] |
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.
Is this intentional? I assume so?
Outside what I commented, everything is looking great. |
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.
In any case, giving this another +1 since it's what was spec'd out, but if we do want to drop declarationMap
that's fine too.
There's a typo that caught my eye in the PR description. The code itself is fine.
It should say |
Edited it. And it should also have said module=nodenext |
Fixes #58420
Sample outputs