- Notifications
You must be signed in to change notification settings - Fork12.9k
Allows emitting buildInfo when --noEmit is specified#39122
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
0c70dd4 toe18dd33CompareThere 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.
Changes look reasonable, although I'm not an expert on how noEmit should interact with buildinfo.
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 like the idea, but I had some trouble understanding the implementation.
| @@ -462,7 +462,6 @@ namespace ts { | |||
| { | |||
| name: "noEmit", | |||
| type: "boolean", | |||
| affectsEmit: 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.
Personally, I find this surprising enough that I'd leave it present but commented out and with an explanation.
| @@ -1013,6 +1013,12 @@ namespace ts { | |||
| } | |||
| } | |||
| function errorSkippedOn(key: keyof CompilerOptions, location: Node | undefined, message: DiagnosticMessage, arg0?: string | number, arg1?: string | number, arg2?: string | number, arg3?: string | number): Diagnostic { | |||
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'm having trouble wrapping my head around what this does and would find an explanation helpful.
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.
location, message, arg0, ... are the existing error logging parameters.
key: keyof CompilerOptions is a typesafe way to assert thatkey, while a string, actually refers to a property inCompilerOptions. That way the string can safely be used as a property name ofCompilerOptions infilterSemanticDiagnostics:
filter(diagnostics, d => !d.skippedOn || !options[d.skippedOn])
This keeps diagnostics that
- were not issued by
errorSkippedOn, so didn't setskippedOn
-OR- - do have
skippedOn, which is guaranteed to be the name of a property in compiler options, say "noEmit". For the "noEmit" example,!options[d.skippedOn]is equivalent to!options.noEmit.
This meta-programming JS would require an enum plus a switch-case conversion in C# I think. Or the meta-programming facilities there, though the result wouldn't be as typesafe as in Typescript.
| @@ -3675,6 +3678,11 @@ namespace ts { | |||
| return { diagnostics, sourceMaps: undefined, emittedFiles, emitSkipped: true }; | |||
| } | |||
| /*@internal*/ | |||
| export function filterSemanticDiagnotics(diagnostic: readonly Diagnostic[], option: CompilerOptions): readonly Diagnostic[] { | |||
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.
should probably be nameddiagnostics andoptions
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.
There's also a typo in the function name.
This change emits buildinfo even if
noEmitis specifiedNow referencing composite project with
noEmitsetting is error instead of erroring on project itself.This allows to typecheck only run on composite projects just like any other project.
Also as part of this change, there were checker errors that were skipped if
noEmitis on. Instead now those errors are generated but filtered when someone asks for semanticDiagnostics instead. This allows to preserver the diagnostics across emit and noEmit builds and having to invalidate cache/emit files/make extra pass on the code to get those diagnosticsFixes#38440