-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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 -b
removes silent noEmitOnError
and emits files even if there are errors
#58838
Conversation
@typescript-bot test it |
Hey @sheetalkamat, the results of running the DT tests are ready. Everything looks the same! |
Oh, I think those test results are actually good, because we're seeing compilation proceed where it couldn't before, right? |
Or, maybe not; the puppeteer case has two tsconfigs for cjs/esm but maybe are getting buildinfo put into the same place? Or something? |
@sheetalkamat Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
78fd963
to
3eda2ed
Compare
@sheetalkamat Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
I ran this locally and the errors are reported because the dependencies are not installed (i think it just clones and reports errors) but i tried this on main as well as this branch and number of errors are same "110" so i am not sure what bot is reporting? If i run "npm run build" at top and then do |
The runner definitely does an install; see: https://typescript.visualstudio.com/TypeScript/_build/results?buildId=162186&view=logs&j=e514356d-f816-5eb0-c695-cd1c85d38e8d&t=87388b4c-e53f-56cd-6000-1001b7a7d293 (starting at line 388) The only weird bit is just the order in which projects are built as far as I can tell. |
…are errors Also add roots property to non incremental build to check if root files went missing
3eda2ed
to
71a06bb
Compare
All right so i reproed it and looked at the diff and it turns out its just order. This is better and correct behavior as the errors will be sorted and reported just like they were with |
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.
Now that there's a .buildinfo
present, we can use it to store the error reporting state and don't have to rely solely on the output timestamps to ensure following builds report an error, allowing us to actually emit even when there are errors. Nice. This is probably the last thing that was blocking allowing all CLI options alongside -b
at the top level, right? Just need to adjust the options parser to allow them as overrides alongside the explicit -b
options and pass them through to the component projects.
src/compiler/builder.ts
Outdated
@@ -1039,7 +983,7 @@ function handleDtsMayChangeOfFileAndExportsOfFile( | |||
function getSemanticDiagnosticsOfFile( | |||
state: BuilderProgramStateWithDefinedProgram, | |||
sourceFile: SourceFile, | |||
cancellationToken: CancellationToken | undefined, | |||
cancellationToken?: CancellationToken, |
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? IIRC the token is usually required as to not forget to pass it around.
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.
Good catch.. that got left out from WIP and fiddling where and how to get the errors
Yes thats a next todo on going through them and selecting which ones make sense. |
Just a note that even with this change project is still not built if upstream has errors and i am looking into relaxing that so we can always generate js files and report errors |
On top of #58626
Add hasErrors property to tsbuildInfo so we can build again if there are errors
Also add roots property to non incremental build to check if root files went missing.