Skip to content
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

Disable noEmitOnError #59223

Merged
merged 1 commit into from
Jul 10, 2024
Merged

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Jul 10, 2024

After #58854, build mode proceeds even if there were errors. However, in our codebase we use noEmitOnError.

If I stick a random type error in src/compiler/types.ts, then run the build, I end up with 17 thousand errors:

src/typingsInstaller/nodeTypingsInstaller.ts:177:22 - error TS2339: Property 'log' does not exist on type 'NodeTypingsInstaller'.

177                 this.log.writeLine(`    Succeeded. stdout:${indent(sys.newLine, stdout)}`);
                         ~~~

src/typingsInstaller/nodeTypingsInstaller.ts:183:18 - error TS2339: Property 'log' does not exist on type 'NodeTypingsInstaller'.

183             this.log.writeLine(`    Failed. stdout:${indent(sys.newLine, stdout)}${sys.newLine}    stderr:${indent(sys.newLine, stderr)}`);
                     ~~~


Found 17418 errors.

This really breaks the self check in CI (like https://github.com/microsoft/TypeScript/actions/runs/9868392724/job/27250345885?pr=59217).

Disable the flag in our codebase, netting:

$ node ./built/local/tsc.js -b ./src
src/compiler/types.ts:10277:14 - error TS2322: Type 'string' is not assignable to type 'number'.

10277 export const oops: number = "string";
                   ~~~~

Found 1 error.

This does make me wonder if noEmitOnError is now just a general hazard when using build mode. Maybe this flag should really enable the old behavior? #58854 (comment)

Or, detect when a dependency has noEmitOnError, had an error, then also refuse to build?

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jul 10, 2024
@jakebailey
Copy link
Member Author

jakebailey commented Jul 10, 2024

@sheetalkamat What do you think of modifying tsc -b to refuse to build if a dependent project had noEmitOnError set and didn't emit? It seems like otherwise, you'll always end up with a very large set of errors...

For people who don't care about the emitted files, removing this flag is still a good idea IMO since you'll get everything anyway, so this PR itself seems like what we want (so I'm going to merge).

@jakebailey jakebailey merged commit 2fd707d into microsoft:main Jul 10, 2024
29 checks passed
@jakebailey jakebailey deleted the disable-no-emit-on-error branch July 10, 2024 16:59
@sheetalkamat
Copy link
Member

It isnt that simple and i had thought about this but i think we should try it out and seek more feedback.
If depends on whether clean build was done or not. because if clean build was done and then introduced error, the rest of the projects most likely wont build at all since d.ts wont change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants