-
Notifications
You must be signed in to change notification settings - Fork 508
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
Comments
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. 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. Another option is to have another command like You can also run
Again that's an opinion... Doesn't matter how many times you repeat it, it's still an opinion. |
I forgot to mention that besides Currently 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 Notably, the |
Well considering
Sorry maybe this wasn't clear -- You pointed out a separate issue here that
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.
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.
Yea, that's unexpected/bad. See
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 |
Not having |
I didn't know this, but rpts2 apparently forces So yea, then you're right, this wouldn't be breaking. Would you like to make a PR to add Would have to add:
The templates can be reconfigured as a user wants, so ideally
I don't disagree that they have lots of features and that in TSDX's case,
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
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
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. |
So, you're a user, you get a type error in a dependency in Moreover, the ambient declarations are typically already checked at authoring time; the reason why
Incremental builds are a specific feature in TS (the 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
|
Options for incremental builds:
|
Are you saying the correct thing to do is to... ignore it??
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 😕
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.
Welp, well that's ironic. One of those things that pop up when it doesn't dogfood itself: #381
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.
Not totally sure what you're trying to say here.
I'm not sure what the purpose of In any case, the requirement of setting I'm still ok with the |
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 Declaring strictness as an end in itself is not a serious approach; it depends on the specifics. The I've found that I should explore alternatives to |
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
We're talking about defaults here. Again, you can change your
Ok, I'm confused -- I thought you meant using
I agree that's not necessary for type-checking, which is why I've said a few times now I would support
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 ( |
It's clearly not a production config since it emits only the declarations (as a cache for the 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 Here's the PR adding 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. |
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.” 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. |
You said this about
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
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 But again, if you need the performance, you can always configure
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. The takeaway I got from it was that It says enable only if needed, not use as a default.
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"?
That's again, not the only way to fix, but #542 is a counter-example
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.
Once again, this is completely configurable, and this merely a default. |
@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.
Thanks for giving another perspective!
👍 👍 👍 Would be great to get TS team's thoughts since I'm getting mixed messages from what I'm seeing.
In a quick summary:
|
The linked PR mentions that lib errors are "generally outside the users' control to fix".
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
It's an example of a spurious error, and it doesn't come from a declaration written by me, but from 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.
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. |
Yes, I understand that and you've said that before. My question was if emitting, why
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.
Ok, so it sounds like you indeed are suggesting that
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.
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:
|
Emitting only the declarations is enough for
I've mentioned that it's also about performance from the outset. The
All
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.
I've given the technical reasoning for what I've said (like why |
This thread winds through a few topics, so I'll give my ideas in short-form
|
We just had the weekly design meeting - which included a discussion on 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. |
@orta thanks so much for chiming in!! Does the team plan to change the
For context, TSDX users are both web and node (including me) and the pipeline is actually TypeScript -> ESNext then Babel ->
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?
I believe @slikts was referring to use it solely for type-checking with
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 |
@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 |
@orta pinging again. |
@orta one more ping |
Yeah, the new plan for I've not yet updated any of the tsconfig settings for 3.9 - that's all |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Looks like this was added to It's not listed as "recommended" in the |
@allcontributors please add @slikts for idea, questions |
I've put up a pull request to add @slikts! 🎉 |
@allcontributors please add @orta for questions, docs |
I've put up a pull request to add @orta! 🎉 |
This is a related issue to #352. The build script works with rollup.js, not
tsc
, so thenoEmit
option should be enabled by default. Users runningnpx tsc
shouldn't expect to have to specify--noEmit
, because usingtsc
is a normal part of TS development. Currentlynpx tsc
will clutter thesrc
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).The text was updated successfully, but these errors were encountered: