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

[Ember]: Set noEmitOnError to false #254

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Feb 26, 2024

Reasons for doing this:

  • we no longer use ember-cli-typescript and recommend against its usage in new projects (the comment in the tsconfig.json here mentioned that the setting for true was to have ember-cli's error reporting screen pick up on type errors)
  • noEmitOnError can break tools in silent ways, including Glint
  • type-checking JS/TS is more treated as a lint these days, rather than compiled code, like Rust
    • additionally, it's usually ok to craft specific lies in certain situations, which other languages don't allow you to do (maybe due to limitations of TS, a library, many reasons)

Other tools that behave similar:

  • CLI tools that compile TS and don't block you when you have an error
    • Vite
    • Vitest
    • Bun
    • ESBuild
    • SWC
    • Rollup (depending on your plugins (using babel, or just not tsc for example)

cc @wagenet @gitKrystan @simonihmig

@orta
Copy link
Member

orta commented Feb 26, 2024

Reasoning seems good to me, but it does need an ember person to give a thumbs up

@boris-petrov
Copy link

I'm not sure I understand why this value matters at all in an Ember app. Code generation is done by Babel, not by TypeScript, right? So this flag should mean nothing. Or am I missing something?

@NullVoxPopuli
Copy link
Contributor Author

Code generation is done by Babel, not by TypeScript, right? So this flag should mean nothing. Or am I missing something?

you are correct, and it really only changes behavior for apps with ember-cli-typescript installed,

the prior comment on the desired behavior from back when we were using ember-cli-typescript:

// --- Compilation/integration settings
// Setting `noEmitOnError` here allows ember-cli-typescript to catch errors
// and inject them into Ember CLI's build error reporting, which provides
// nice feedback for when

without ember-cli-typescript, ember-cli doesn't know when there are type errors, so the change in this PR is mostly to benefit library authors.

@gitKrystan
Copy link
Collaborator

This change seems good. Since the default is false anyway, should we just remove the config altogether?

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Feb 26, 2024

should we just remove the config altogether?

unsure, I worry that not documentating decisions could lead us to repeating history

@gitKrystan gitKrystan merged commit 8fee712 into tsconfig:main Feb 26, 2024
1 check passed
@NullVoxPopuli NullVoxPopuli deleted the patch-3 branch February 26, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants