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

noEmit and skipLibCheck should be on by default in TS compiler options #529

Closed
slikts opened this issue Mar 2, 2020 · 31 comments · Fixed by #864
Closed

noEmit and skipLibCheck should be on by default in TS compiler options #529

slikts opened this issue Mar 2, 2020 · 31 comments · Fixed by #864
Labels
kind: feature New feature or request kind: optimization Performance, space, size, etc improvement scope: templates Related to an init template, not necessarily to core (but could influence core)

Comments

@slikts
Copy link

slikts commented Mar 2, 2020

This is a related issue to #352. The build script works with rollup.js, not tsc, so the noEmit option should be enabled by default. Users running npx tsc shouldn't expect to have to specify --noEmit, because using tsc is a normal part of TS development. Currently npx tsc will clutter the src directory with transpiled files, which can be difficult to clean up if there are uncommitted changes mixed in.

This is important because without tsc there is no way to do project-wide type checking. The build script will only stop at the first error. The test runner will report errors, but only for files that are tested, and it's also the wrong place for type checking (#521).

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 2, 2020

Do you mean turning it on by default in the templates? Or in the build command itself?

I think your intent here is to have type-checking separated from build (or test), which makes sense to an extent.
But changing tsconfig.compilerOptions.noEmit to true would be very unexpected. The build command should emit by default, not just report type errors by default -- that is how build tools work after all, including tsc itself.

In either case, that would be a breaking change. I think #352 makes more sense than this, but #352's change doesn't actually align with how ESLint works.
An intermediate change would be to change the templates such that the lint script runs tsc --noEmit && tsdx lint. That way a workflow isn't being forced upon users (they can change it/it would be merely a recommendation) and tsdx lint isn't different from ESLint's behaviors.
Misalignment with the underlying tools & their integrations is already an increasing issue with TSDX (#514, #498, some on testing too), and that misalignment is very confusing and unintuitive, so I don't think further misaligning by changing tsdx lint itself is a good idea.

Another option is to have another command like tsdx tsc, but I have a feeling that would cause more issues than it solves.

You can also run tsdx watch --noEmit for something similar to what you're looking for.

it's also the wrong place for type checking (#521).

Again that's an opinion... Doesn't matter how many times you repeat it, it's still an opinion. ts-jest exists and has its defaults as such for a reason.

@slikts
Copy link
Author

slikts commented Mar 3, 2020

tsc is primarily a type checking tool that can also transpile. I don't see how it emitting transpiled files into the source directory (not the build directory) would be useful, particularly when the actual build uses Rollup.js instead, or for what users it would be a breaking change.

I forgot to mention that besides noEmit it also requires skipLibCheck, since otherwise tsc can fail when checking dependencies in node_modules.

Currently tsdx build fails on the first error, so if you want to fix all the errors so that the build would pass, you need to run tsc. Without noEmit, it's essentially a trap, since it creates many files in the source directory, requiring something like git clean -f to resolve.

There are also type errors in the test runner, but not necessarily for all files. It's an opinion that the test runner should just run tests instead of checking types, but it's well-grounded, so I'm repeating it.

Currently, to check all the source files for type errors, it requires knowing to run tsc with one or two flags, otherwise it will create an inconvenience for the user or possibly fail with a false positive.

Notably, the create-react-app TS template has noEmit and skipLibCheck enabled.

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 3, 2020

tsc is primarily a type checking tool that can also transpile

Well considering --noEmit is a flag, I don't think the TypeScript team would agree.

I don't see how it emitting transpiled files into the source directory (not the build directory) would be useful, particularly when the actual build uses Rollup.js instead, or for what users it would be a breaking change.

Sorry maybe this wasn't clear -- tsconfig.json configures both TSDX and tsc. That's where the issue is.
That's also why I asked if you meant templates or build command itself. Just changing the templates' tsconfig.json means that tsdx build would also not emit by default.
We can override tsdx build to always emit and then set the templates to have noEmit, but that override would be breaking to users that configure noEmit for TSDX. It's probably not a huge number of people, but it is breaking.

You pointed out a separate issue here that outDir should probably be set to ./dist/ by default in the templates. Though TSDX doesn't currently handle that config option too well (there's a few issues on that and #367 that still hasn't been reviewed) so it's not really "configurable" right now.

it also requires skipLibCheck, since otherwise tsc can fail when checking dependencies in node_modules

I don't actually have experience with this flag, but afaict this skips checking all declaration files, including ones you may have created yourself. You'd want the ones you created to be checked.

Currently tsdx build fails on the first error, so if you want to fix all the errors so that the build would pass, you need to run tsc

Yea you mentioned this and I guess I never really noticed this. It sounds like it would be good if outputted all errors, so I'll look into that.

Without noEmit, it's essentially a trap, since it creates many files in the source directory, requiring something like git clean -f to resolve.

Yea, that's unexpected/bad. See outDir comment above. That would fix this issue.

Notably, the create-react-app TS template has noEmit and skipLibCheck enabled.

This is a good example, especially since TSDX has borrowed pieces of CRA before. I don't know how long it's had those options; there might be a reason why Jared didn't stick with them. I know CRA reads tsconfig.json (#489 uses similar code) but I'm not sure how much of it is overridden if any (particularly for dev vs. prod builds). That may be where the difference is, will have to look.

@slikts
Copy link
Author

slikts commented Mar 3, 2020

noEmit has no effect on tsdx build because tsdx uses Rollup (rollup-plugin-typescript2 just checks types and transpiles). tsdx build completes fine with noEmit set to true. outDir should not be set and insted emitting with tsc should be disabled because tsc is not used as the build tool, but Rollup is. Rollup or webpack are specialized build tools and so are preferable to tsc because of all the features and plugins they have. That's why tsc is primarily a type checker that can also transpile, but it isn't used for transpiling in more advanced contexts.

skipLibCheck being disabled makes type checking fail if there are type errors in dependencies. Having to fix dependencies in node_modules to pass type checks is almost never what the user would want, and it's also not what tsdx build currently does. tsdx build also doesn't check the ambient declarations.

Not having skipLibCheck also makes builds unnecessarily slower; just tsc --skipLibCheck on my test project takes 3s, just tsc takes 10s, and tsdx build takes 30s. This is also why type checking shouldn't be coupled to building, because it delays feedback.

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 3, 2020

noEmit has no effect on tsdx build because tsdx uses Rollup (rollup-plugin-typescript2 just checks types and transpiles). tsdx build completes fine with noEmit set to true.

I didn't know this, but rpts2 apparently forces noEmit to false. I thought turning it to true would be the equivalent of a dry-run but I guess it's just ignored.

So yea, then you're right, this wouldn't be breaking. Would you like to make a PR to add noEmit to the templates?

Would have to add:

  • "noEmit": true to each template
  • a comment next to this option saying something like // this is only for type-checking with tsc, tsdx build forces this to false
  • possibly a typecheck script? (and related docs). would just run tsc

outDir should not be set and insted emitting with tsc should be disabled because tsc is not used as the build tool, but Rollup is.

The templates can be reconfigured as a user wants, so ideally outDir should be set as well so that if they remove noEmit or something they don't fall into the trap. Also because I'm working on making the ability to change outDir.

Rollup or webpack are specialized build tools and so are preferable to tsc because of all the features and plugins they have. That's why tsc is primarily a type checker that can also transpile, but it isn't used for transpiling in more advanced contexts.

I don't disagree that they have lots of features and that in TSDX's case, tsc would just be a type checker. But lots of projects use tsc directly to compile, and even Rollup users use tsc: ezolenko/rollup-plugin-typescript2#189 . This is off-topic in any case.

skipLibCheck being disabled makes type checking fail if there are type errors in dependencies. Having to fix dependencies in node_modules to pass type checks is almost never what the user would want

facebook/create-react-app#5738 (comment) confirms that all type declaration files get skipped if this is disabled, including internal ones. I'm not sure I agree with the position that a user would "almost never" want this. If there are type errors in your dependencies, that's a problem and you should fix that (by upgrading or overriding etc). TSDX itself has an internal declaration file. This file wouldn't be type-checked if skipLibCheck is disabled.

it's also not what tsdx build currently does. tsdx build also doesn't check the ambient declarations.

Huh, weird I just confirmed that myself. It's not mentioned in rpts2's docs, so this might be a bug actually. EDIT: Filed ezolenko/rollup-plugin-typescript2#212

Not having skipLibCheck also makes builds unnecessarily slower; just tsc --skipLibCheck on my test project takes 3s, just tsc takes 10s,

I'm not sure I would characterize this as "unnecessarily" slower, it checks things that might not be correct, including any of your own declarations.
It is a big hit, but shouldn't be big overall when watching for incremental changes during development, and for production it seems quite useful.

@slikts
Copy link
Author

slikts commented Mar 4, 2020

So, you're a user, you get a type error in a dependency in node_modules that you don't control, and your build fails; what are you supposed to do? There wouldn't even necessarily be a straightforward patch; it might be a conflict in complex typings, or an issue with generated files (meaning that you would have to delve into the build setup of some other project to fix it, or patch generated files), etc. Remember that this makes the build fail on top of being slower.

Moreover, the ambient declarations are typically already checked at authoring time; the reason why skipLibCheck exists is because the declarations are typically already known to be correct and don't need to be checked every time.

… shouldn't be big overall when watching for incremental changes …

Incremental builds are a specific feature in TS (the incremental compiler option), and they would require noEmit to be false, and emitDeclarationOnly with a temporary outDir (that's also in .gitignore). I didn't propose it since it's more complex (the outDir needs to be unused, for example, to avoid conflicts), but it might be a good idea; it's what I use myself.

The other kind of incremental checking is watch mode, but that's only appropriate when working on something for an extended period, and even then it should be normal to check the types for the rest of the project manually, since the editor would already check the files being edited.

Your example declaration file in tsdx has essentially nothing in there to be checked (it's actually broken, but in an inconsequential way; it's not detected because tsdx itself uses skipLibCheck). For the projects that do something advanced in ambient declarations, there are special tools like tsd or dtslint. Those are advanced use cases, however.

skipLibCheck also makes the type checking noticeably slower; it slows it down several-fold for the projects that I've timed it with.

@slikts
Copy link
Author

slikts commented Mar 4, 2020

Options for incremental builds:

    "incremental": true,
    "emitDeclarationOnly": true,
    "outDir": ".cache/tsbuild",
    "skipLibCheck": true,

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 4, 2020

So, you're a user, you get a type error in a dependency in node_modules that you don't control, and your build fails; what are you supposed to do?

Are you saying the correct thing to do is to... ignore it??
skipLibCheck makes for less strict typing and TSDX currently has tsconfig.json set to most strict (which I believe is optimal)
I think it makes more sense to either explicitly override the types or patch or upgrade to a newer version or file an issue or check if there is a workaround available.

Moreover, the ambient declarations are typically already checked at authoring time; the reason why skipLibCheck exists is because the declarations are typically already known to be correct and don't need to be checked every time.

The keyword here is typically. An author may also have less strict typings than you might. And they may have hand-written the declarations and didn't check them. Which, as you pointed out, is something TSDX itself is doing 😕

Your example declaration file in tsdx has essentially nothing in there to be checked

It's really easy to accidentally have a type error when hand-writing declarations. And it wouldn't be checked. This problems in the internal declaration are pretty much that exactly.

(it's actually broken, but in an inconsequential way; it's not detected because tsdx itself uses skipLibCheck)

Welp, well that's ironic. One of those things that pop up when it doesn't dogfood itself: #381

skipLibCheck also makes the type checking noticeably slower; it slows it down several-fold for the projects that I've timed it with.

Based on my reading, that's the primary reason it exists, yes. I don't think trading off strictness for a performance optimization should be the default case. You can always re-configure it, but strictness should be default.


and even then it should be normal to check the types for the rest of the project manually, since the editor would already check the files being edited.

Not totally sure what you're trying to say here.
My point was that the performance penalty will only happen on the first build if using any form of incremental checking, and watch mode is the default script for yarn start.

Options for incremental builds:

I'm not sure what the purpose of emitDeclarationOnly and skipLibCheck are here? They're not necessary for incremental to be used.

In any case, the requirement of setting outDir makes this difficult to have as a default since outDir will be configurable soon inside of TSDX and is already used by rpts2. Would need to have something like split dev/prod tsconfigs.


I'm still ok with the noEmit option being added in a PR as I said previously. The rest of this is kind of a separate issue.

@slikts
Copy link
Author

slikts commented Mar 5, 2020

Are you saying the correct thing to do is to... ignore it??

If the build is failing due to external typings, the main actionable fix will be to disable checking them, and it particularly makes sense since the errors may be spurious and/or have no impact (like the tsdx declaration error, or the errors that allowSyntheticDefaultImports was created to ignore). Filing an issue or looking for upgrades in many cases will amount to wishful thinking.

Declaring strictness as an end in itself is not a serious approach; it depends on the specifics.

The incremental compiler option requires emitting, and since the actual build artifacts are created by Rollup, emitDeclarationOnly is a way to cache just the declarations, and I included skipLibCheck since it's part of the difference from the current config. yarn start doesn't factor into it since it's not suitable for type checking due to being slower and incomplete. Not having to wait unnecessarily for common tasks is a significant part of a good developer experience.

I've found that I should explore alternatives to tsdx that would be guided by better experienced input.

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 6, 2020

If the build is failing due to external typings, the main actionable fix will be to disable checking them

That is one possible fix, not the only one. And it is a very overbroad one at that, as it disables checking all declaration files. And, again you can change your tsconfig.json to disable skipLibCheck if that's the solution you want to use.

Declaring strictness as an end in itself is not a serious approach; it depends on the specifics.

We're talking about defaults here. Again, you can change your tsconfig.json however you want.

The incremental compiler option requires emitting, and since the actual build artifacts are created by Rollup, emitDeclarationOnly is a way to cache just the declarations

Ok, I'm confused -- I thought you meant using incremental as a way to speed up type-checking with tsc not actual production builds.

Not having to wait unnecessarily for common tasks is a significant part of a good developer experience.

I agree that's not necessary for type-checking, which is why I've said a few times now I would support noEmit and possibly a typecheck script.

I've found that I should explore alternatives to tsdx that would be guided by better experienced input.

Ok sure. Again, you can reconfigure TSDX however you want. The defaults don't seem to align with your opinions, but those are, again, opinions, and again, at least some of these don't seem to be widely supported by other users (skipLibCheck I have seen a good bit, but I'm not sure it's an optimal default).
Also the defaults for most users, particularly beginners, may not necessarily be the best for very advanced users (who likely have their own configs), those are two different workflows. What's optimal for you is not necessarily optimal for someone else.

@slikts
Copy link
Author

slikts commented Mar 6, 2020

Ok, I'm confused -- I thought you meant using incremental as a way to speed up type-checking with tsc not actual production builds.

It's clearly not a production config since it emits only the declarations (as a cache for the incremental option to work), and the outDir is in .cache.

As for opinions: not all opinions are the same; they differ by being well-grounded and informed or not. Even facts require interpretation. I'm not saying anything that would be akin to stating my favorite color; it's about what would be common scenarios for the user. Having faster type checking and not having builds break because of reasons outside of the user's control benefits the common case. Having complex types in ambient declarations is an advanced case. The suggestion that skipLibCheck would be optimal for advanced cases or specific to my workflow is the opposite of what's being said.

Here's the PR adding skipLibCheck to CRA that states the same rationale, and it also links to issues where users ask for it due to performance reasons. I found one comment in their repo where a user turned on skipLibCheck due to needing to check the ambient declarations.

The TS repo has many examples of complex issues with 3rd party typings (random example). Here's a good SO answer detailing the reasons for it. The issue highlights type conflicts (different dependencies typing shared APIs), but there can be other issues; here's an odd error from my own library; it's unclear how to fix it since it uses a standard config and doesn't do anything special.

I'm looking at alternatives since these discussions are unnecessarily taxing; it shouldn't need to be explained that strictness shouldn't be an end in itself, particularly not in the context of popular tooling in a complex ecosystem.

@jaredpalmer
Copy link
Owner

This is a really good discussion. Learned a lot about TS!

A bit of history, when I made TSDX I made one change to how Formik’s build system worked: rpts2. OG pre-tsdx Formik used to use a two-step build (because Babel TS didn’t really exist yet). First it would use tsc and emit esnext js to a temp directory. Then rollup would bundle the temp directory to dist (so it could use Babel plugins). It’s very possible that I incorrectly kept around some TS config options that became redundant by switching to rpts2 but never noticed because “it just worked.”

FWIW, my course of action with libraries and apps differs with respect to incorrect dependency types. When writing an app, I will 100% use patch-package to fix the types. I want to check my deps. During dev/watch, I’m fine with foregoing dep checks on large projects, but prefer it if possible. During builds, I want as much type safety as possible. When making a lib, however, I don’t think patch package is acceptable. So I usually will fork the dep and publish my own corrected version or copy it into my libs source (with proper correction) if it’s small enough. Until reading this thread, I never considered skiplibCheck for libs, but maybe I should have. I usually err on the side of strictness and correctness over build performance.
—-
I think the question we want to answer is what the default behavior should be for novice and intermediate typescript folks. We will obviously have to document this somewhere in an “advanced config” section

cc’ing typescript team for their suggestion @orta

tl;dr we are debating what defaults should be for noEmit, skiplibCheck, incremental builds, and emitDeclaration given how rollup-plugin-typescript2 works.

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 12, 2020

It's clearly not a production config since it emits only the declarations (as a cache for the incremental option to work), and the outDir is in .cache.

You said this about emitDeclarationOnly:

and since the actual build artifacts are created by Rollup, emitDeclarationOnly is a way to cache just the declarations

If it's not for production anyway, then why cache just the declarations? Can't you just cache all the dev artifacts? Or are you saying incremental type-checking is faster when using emitDeclarationOnly? Sorry, it's not clear what you meant, hence confusion
I'd think there's more metadata for perf reasons when producing everything, but idk how it works under-the-hood and have not tested this. You didn't specify, so I'm not sure.

Here's the PR adding skipLibCheck to CRA that states the same rationale, and it also links to issues where users ask for it due to performance reasons.

Thanks for providing an example. The main rationale there is performance though (not ignoring errors) and the central topic of facebook/create-react-app#5820 is actually around async for ForkTsCheckerWebpackPlugin, which couldn't be configured externally, unlike skipLibCheck.
Async type-checking would be good to have here too -- there is #151 for that and the upstream issue in rpts2.

But again, if you need the performance, you can always configure skipLibCheck to false. Nothing is preventing anyone from doing that. And again, incremental/watch performance shouldn't suffer much on average from it being true.
TSDX users so far have rarely complained about performance, so I think the priorities differ here. For library builds, I would certainly at least think that safety outweighs build performance.

The TS repo has many examples of complex issues with 3rd party typings (random example). Here's a good SO answer detailing the reasons for it. The issue highlights type conflicts (different dependencies typing shared APIs), but there can be other issues

Yea, I tried to do my research on it and have seen many of these myself, so I understand this issue has pretty broad scope in the community.
I've read the exact same SO answer and notably that one says:
"--skipLibCheck degrades type checking, and ideally we wouldn't use it. But not every library provides perfect types yet, so skipping it can be nice."

The takeaway I got from it was that It says enable only if needed, not use as a default.

here's an odd error from my own library; it's unclear how to fix it since it uses a standard config and doesn't do anything special.

That is pretty weird and I see how that's an issue (might be worth filing with TS). Though your library seems to be written in TS and has a type issue when used, which seems to contradict the assertion that everything is ok "at authoring time"?

Filing an issue or looking for upgrades in many cases will amount to wishful thinking.

That's again, not the only way to fix, but #542 is a counter-example

I'm looking at alternatives since these discussions are unnecessarily taxing;

Not for nothing, but I am an unpaid volunteer and you've made many remarks that I feel are condescending. I've been trying extremely hard not to respond to that and did not want to respond negatively back to you. I even stepped away from responding to your issues because they were negatively impacting my mental health.

You also haven't exactly made it easy to accept your opinions on top of that. If you immediately provided lots of evidence and statements from the community, it'd be much easier to take your side.
Some of the examples you provide contradict your own opinion and you've made some faulty comparisons. Some of these issues aren't well supported either, e.g. your ts-jest issue has 0 up-votes after being around for 1.5 years, and the TSDX issue has actually been down-voted. These make me much more cautious.
In contrast, the CRA issue had 45+ up-votes.

it shouldn't need to be explained that strictness shouldn't be an end in itself, particularly not in the context of popular tooling in a complex ecosystem.

Once again, this is completely configurable, and this merely a default. tsc --init does not set skipLibCheck: true either. That is the most popular tool. And it does set strict: true. TypeScript was built for safety and its defaults err on the side of safety.

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 12, 2020

It’s very possible that I incorrectly kept around some TS config options that became redundant by switching to rpts2 but never noticed because “it just worked.”

@jaredpalmer this is more around missing options and a few of these are dev/type-check mode only. But #538 has a bit on options in the example template that might make sense to change.

skipLibCheck not working in rpts2 is the topic of ezolenko/rollup-plugin-typescript2#212.
The big issues with the config have actually historically been around things like target , paths, and rootDir 😅

FWIW, my course of action with libraries and apps differs with respect to incorrect dependency types. [...]

Thanks for giving another perspective!

I think the question we want to answer is what the default behavior should be for novice and intermediate typescript folks. We will obviously have to document this somewhere in an “advanced config” section [...]

👍 👍 👍 Would be great to get TS team's thoughts since I'm getting mixed messages from what I'm seeing.

tl;dr we are debating what defaults should be for noEmit, skiplibCheck, incremental builds, and emitDeclarationOnly given how rollup-plugin-typescript2 works.

In a quick summary:

  • noEmit totally makes sense in TSDX's case for type-checking only, since that's the only thing you'd be using tsc for
  • incremental and emitDeclarationOnly are also around dev/type-check mode as far as I understand. Which is faster? Also incremental vs. watch mode
  • skipLibCheck is the big debate here, for both prod and dev mode

@slikts
Copy link
Author

slikts commented Mar 12, 2020

Can't you just cache all the dev artifacts? Or are you saying incremental type-checking is faster when using emitDeclarationOnly? Sorry, it's not clear what you meant, hence confusion

tsc requires emitting for incremental to work, otherwise it errors out:

Option 'noEmit' cannot be specified with option 'incremental'.ts

The only rationale there is performance though …

The linked PR mentions that lib errors are "generally outside the users' control to fix".

The takeaway I got from it was that It says enable only if needed …

The defaults for a preset meant for the general use case depend on what the general case use is, and the answer doesn't comment on that.

The general case arguably is that users won't author complex ambient types, and that errors in library types can be spurious, and that enabling skipLibCheck will typically be the solution in case of any library errors that makes the most sense.

Though your library seems to be written in TS and has a type issue when used, which seems to contradict the assertion that everything is ok "at authoring time"?

It's an example of a spurious error, and it doesn't come from a declaration written by me, but from tsc output.

What I meant by declarations being checked at authoring time is that people writing complex ambient types (like for Definitely Typed) wouldn't do it without type checking.

That's again, not the only way to fix, but #542 is a counter-example

The point is that the user's build would be breaking, and even if they filed an issue, the fix would be delayed and not guaranteed. In this particular example the type error also was trivial and had no actual impact, but would still have broken the build.

As for the rest, a lot of the confusion could have been allayed by taking a more charitable or careful reading and holding off on guesswork.

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 12, 2020

tsc requires emitting for incremental to work, otherwise it errors out:

Yes, I understand that and you've said that before. My question was if emitting, why emitDeclarationOnly instead of emitting everything.
You said to set emitDeclarationOnly because tsdx build has its own build process, but those are separate cases, one is for dev type-checking and the other production builds.

The linked PR mentions that lib errors are "generally outside the users' control to fix".

Sorry, I should've used the word "main". The issue is focused on perf (primarily with async mode) and the subject of the PR is perf. As in, your focus seems flipped from theirs.

The general case arguably is that users won't author complex ambient types, and that errors in library types can be spurious, and that enabling skipLibCheck will typically be the solution in case of any library errors that makes the most sense.

What I meant by declarations being checked at authoring time is that people writing complex ambient types (like for Definitely Typed) wouldn't do it without type checking.

Ok, so it sounds like you indeed are suggesting that skipLibCheck should be enabled for the vast majority of usage, and only enabled for DT authors and the like.
This despite that there being no guarantee the typings aren't buggy, that there are independent tools for that, and that tsc's default is the opposite. I don't think the SO answer or tsc are aimed primarily at DT authors.
I understand what you're saying about spurious errors, perf, etc, but there's a big misalignment there. A TS team member could clarify that.

The point is that the user's build would be breaking, and even if they filed an issue,

All I had to do was upgrade patch versions to get a fix. And, as I've said before and as with Jared's workflow, upgrading and filing issues are not the only solutions.

could have been allayed by taking a more charitable or careful reading and holding off on guesswork.

Yea, I don't know why I'm responding, this is not healthy. Continued condescending remarks are abusive. I did not call other libraries legacy when they are not, did not repeatedly suggest others didn't know anything, didn't read, or were making guesses, did not repeatedly say other opinions were "wrong", and didn't make puns at others' expense, among other such comments:

Nominally, but CRA has a significantly better TS DX.

@slikts
Copy link
Author

slikts commented Mar 12, 2020

Yes, I understand that and you've said that before. My question was if emitting, why emitDeclarationOnly instead of emitting everything.

Emitting only the declarations is enough for tsc to work with incremental.

You said to set emitDeclarationOnly because tsdx build has its own build process, but those are separate cases, one is for dev type-checking and the other production builds.

tsdx build should obviously override emitDeclarationOnly to emit the full build instead of just the declarations.

As in, your focus seems flipped from theirs.

I've mentioned that it's also about performance from the outset. The incremental compiler option was added some months later than the CRA changes, and it's a cleaner solution to the performance issue, so that leaves the focus on errors in dependencies.

This despite that there being no guarantee the typings aren't buggy, that there are independent tools for that …

All @types packages, for example, are checked before publishing, so there are some guarantees there. Conflicts are a different matter, though.

And, as I've said before and as with Jared's workflow, upgrading and filing issues are not the only solutions.

The other solutions are involved and situational. You can make a fork, which you'd also then have to maintain, and which would require dealing with the other project's build setup, which may take manual steps and reading, and may have quirks. patch-package is not appropriate for libraries, just for apps, and it also increases the maintenance cost. skipLibCheck will be the go-to solution for most cases when the user just wants their build to pass, and when the error has no actual impact (like in the actual concrete examples mentioned so far).

Yea, I don't know why I'm responding, this is not healthy. Continued condescending remarks are abusive. I did not call other libraries legacy when they are not, did not repeatedly suggest others didn't read or were making guesses, and didn't make puns at others' expense, among other such comments: …

I've given the technical reasoning for what I've said (like why ts-jest should be seen as legacy in light of babel-jest, and why CRA provides better developer experience for apps), and also why it matters from the user's perspective. The confusion that this has engendered is not justified, however, and is in itself condescending, since the operating assumption seems to be that I've been unclear. The focus should be on what the use cases would be, and how tooling would improve the workflow for them, and not on personalia, so I'd ask not to direct more queries at me.

@orta
Copy link
Contributor

orta commented Mar 12, 2020

This thread winds through a few topics, so I'll give my ideas in short-form

  • I don't think skipLibCheck should be enabled for users by default, personally. The flag explicitly breaks some of the type system contracts and is available if you need it, but that should be edge-cases not for everyone.

  • I'm a believer in using babel for transpiling and ts for type-checking when dealing with web. As a pragmatist, you probably already have babel in the tree, and it keeps build tools consistent. I'm personally against having types checking happening in tests, because I wouldn't want them blocking running a test. Whether using ts-jest or ts-babel is generally

  • WRT incremental / build mode etc - these are useful for when TS is emitting the .js files in composite projects - which probably isn't happening in your watch mode which needs to go through rollup?

  • the OP on noEmit makes sense to me, given that it would be surprising to me too to see a bunch of ts files.

@orta
Copy link
Contributor

orta commented Mar 13, 2020

We just had the weekly design meeting - which included a discussion on skipLibCheck - the gist is that the team has switched to generally recommend skipLibCheck as true to folks. We can't change the default behind the scenes though, that'd be a breaking change.

The reasoning is that most of the errors it handles for you are generally covered by the DefinitelyTyped automation test suites, and times you might hit the issues it will be quite obvious.

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 13, 2020

@orta thanks so much for chiming in!! Does the team plan to change the tsc --init default (as that's not breaking)? Similar to how esModuleInterop defaults to true there.

  • I'm a believer in using babel for transpiling and ts for type-checking when dealing with web

For context, TSDX users are both web and node (including me) and the pipeline is actually TypeScript -> ESNext then Babel -> preset-env (+ plugins), so we use both.

  • I'm personally against having types checking happening in tests, because I wouldn't want them blocking running a test. Whether using ts-jest or ts-babel is generally

Thanks for sharing. As we've had some discussions on workflow, I am curious what workflow you'd recommend for general use library development. Running tests in one terminal and type-checking in a separate one? Something else?

  • WRT incremental / build mode etc - these are useful for when TS is emitting the .js files in composite projects - which probably isn't happening in your watch mode which needs to go through rollup?

I believe @slikts was referring to use it solely for type-checking with tsc, but I'm not totally sure. tsdx watch indeed goes through rollup.

  • the OP on noEmit makes sense to me, given that it would be surprising to me too to see a bunch of ts files.

I think we're all in agreement on that front 👍 I'm getting the sense that we should also add comments to some of our defaults in general to be more explicit for users

@agilgur5 agilgur5 added the kind: discussion Discussion on changes to make label Mar 13, 2020
@agilgur5 agilgur5 added this to the v0.13.1 milestone Mar 20, 2020
@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 21, 2020

@orta any word on the above?

Would like to get this change in soon and would be good to be able to reference a TS issue or tsc --init with regard to skipLibCheck.

@agilgur5 agilgur5 changed the title noEmit should be on by default in TS compiler options noEmit and skipLibCheck should be on by default in TS compiler options Mar 21, 2020
@agilgur5
Copy link
Collaborator

@orta pinging again.
I found microsoft/TypeScript#37389, but that specifically says "not to turn it on by default" and doesn't elaborate on the discussions around what the default should be.
The v2 docs site on tsconfig also doesn't give any "Recommended" setting for skipLibCheck (e.g. strict has "Recommended True")

@agilgur5
Copy link
Collaborator

agilgur5 commented Apr 5, 2020

@orta one more ping

@orta
Copy link
Contributor

orta commented Apr 5, 2020

Yeah, the new plan for --init is to be this: microsoft/TypeScript#37361 (comment) - not sure if we'll get it in for 3.9 now though. Lots of bug fixes to do.

I've not yet updated any of the tsconfig settings for 3.9 - that's all

@agilgur5 agilgur5 added the scope: templates Related to an init template, not necessarily to core (but could influence core) label Apr 12, 2020
@dgreene1

This comment has been minimized.

@agilgur5

This comment has been minimized.

@agilgur5
Copy link
Collaborator

Looks like this was added to tsc --init in microsoft/TypeScript#37808 and is now in the "recommended" tsconfig base (@tsconfig/recommended) so will be adding this to TSDX's templates as well.

It's not listed as "recommended" in the tsconfig reference though, adding a PR for that momentarily

@agilgur5
Copy link
Collaborator

@allcontributors please add @slikts for idea, questions

@allcontributors
Copy link
Contributor

@agilgur5

I've put up a pull request to add @slikts! 🎉

@agilgur5
Copy link
Collaborator

@allcontributors please add @orta for questions, docs

@allcontributors
Copy link
Contributor

@agilgur5

I've put up a pull request to add @orta! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New feature or request kind: optimization Performance, space, size, etc improvement scope: templates Related to an init template, not necessarily to core (but could influence core)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants